Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW] Fix concurrent JSON reads crash #6003

Merged
merged 6 commits into from
Aug 18, 2020

Conversation

vuule
Copy link
Contributor

@vuule vuule commented Aug 17, 2020

Closes: #5934

Root cause:
A temporary unique_ptr to a mutable_table_device_view object was being passed to device_scalar. The error only reproes if a concurrent thread/process changes the content at the temporary location - no repro with a singlethreaded read.

Fix:
This PR contains a simple fix, and a few fixes for incorrect stream use in the JSON reader.

@vuule vuule requested a review from a team as a code owner August 17, 2020 18:47
@vuule vuule requested review from jrhemstad and codereport August 17, 2020 18:47
@vuule vuule self-assigned this Aug 17, 2020
@vuule vuule added the cuIO cuIO issue label Aug 17, 2020
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@vuule vuule changed the title [WIP] Fix a dangling pointer issue in JSON reader [WIP] Fix concurrent JSON reads crash Aug 17, 2020
@vuule vuule changed the title [WIP] Fix concurrent JSON reads crash [REVIEW] Fix concurrent JSON reads crash Aug 17, 2020
@vuule vuule added the bug Something isn't working label Aug 17, 2020
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve since this is a release blocker and I know cuIO code is going to undergo a refactor in the near future, but passing a view type by pointer is smelly.

@vuule
Copy link
Contributor Author

vuule commented Aug 17, 2020

I'll approve since this is a release blocker and I know cuIO code is going to undergo a refactor in the near future, but passing a view type by pointer is smelly.

Changed the parameter to thrust::optional.

@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #6003 into branch-0.15 will decrease coverage by 4.10%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.15    #6003      +/-   ##
===============================================
- Coverage        88.65%   84.54%   -4.11%     
===============================================
  Files               57       81      +24     
  Lines            10945    13845    +2900     
===============================================
+ Hits              9703    11705    +2002     
- Misses            1242     2140     +898     
Impacted Files Coverage Δ
python/cudf/cudf/core/dtypes.py 88.39% <0.00%> (-8.54%) ⬇️
python/cudf/cudf/core/abc.py 91.48% <0.00%> (-2.96%) ⬇️
python/cudf/cudf/core/frame.py 89.76% <0.00%> (-1.42%) ⬇️
python/cudf/cudf/utils/ioutils.py 86.13% <0.00%> (-1.21%) ⬇️
python/dask_cudf/dask_cudf/sorting.py 93.12% <0.00%> (-0.99%) ⬇️
python/cudf/cudf/core/indexing.py 95.22% <0.00%> (-0.68%) ⬇️
python/cudf/cudf/utils/dtypes.py 85.79% <0.00%> (-0.64%) ⬇️
python/cudf/cudf/core/column/column.py 87.04% <0.00%> (-0.33%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.25% <0.00%> (-0.13%) ⬇️
python/cudf/cudf/core/column/categorical.py 93.61% <0.00%> (-0.10%) ⬇️
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d1f862...2c22d23. Read the comment docs.

@vuule
Copy link
Contributor Author

vuule commented Aug 17, 2020

rerun tests

1 similar comment
@galipremsagar
Copy link
Contributor

rerun tests

@harrism harrism added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Aug 18, 2020
@kkraus14
Copy link
Collaborator

rerun tests

@kkraus14 kkraus14 merged commit f7fbc11 into rapidsai:branch-0.15 Aug 18, 2020
@vuule vuule deleted the bug-json-dangling-ptr branch August 18, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working cuIO cuIO issue
Projects
None yet
7 participants