Skip to content

Fix race condition that caused fread to occasionally read the data only partially#2264

Merged
mattdowle merged 1 commit intomasterfrom
fread2260
Jul 8, 2017
Merged

Fix race condition that caused fread to occasionally read the data only partially#2264
mattdowle merged 1 commit intomasterfrom
fread2260

Conversation

@st-pasha
Copy link
Copy Markdown
Contributor

@st-pasha st-pasha commented Jul 8, 2017

The original code iterated over nJumps + nth to allow all threads one more iteration to have a chance to push the last buffer upstream. The problem with this approach is that there is no guarantee that OMP would distribute these extra nth iterations 1 per each running thread. Although this happens most of the time, occasionally some thread would consume several of the extra iterations, which means that some other thread would not have a chance to push its last buffer.

To fix this problem, we should just push the buffers explicitly after the loop ends.

Not adding any tests, because my measurements were showing the problem only with probability around 0.03%, and may take up to 10 minutes to run. This seems prohibitively expensive...

Closes #2260

@st-pasha st-pasha added this to the v1.10.6 milestone Jul 8, 2017
@st-pasha st-pasha self-assigned this Jul 8, 2017
@st-pasha st-pasha requested a review from mattdowle July 8, 2017 03:58
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 8, 2017

Codecov Report

Merging #2264 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2264      +/-   ##
==========================================
- Coverage   90.81%   90.79%   -0.03%     
==========================================
  Files          59       59              
  Lines       11531    11536       +5     
==========================================
+ Hits        10472    10474       +2     
- Misses       1059     1062       +3
Impacted Files Coverage Δ
src/fread.c 93.52% <100%> (+0.03%) ⬆️
src/bmerge.c 94.97% <0%> (-1.96%) ⬇️
src/forder.c 94.47% <0%> (+0.52%) ⬆️

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 c4b03bf...f35bcb3. Read the comment docs.

Copy link
Copy Markdown
Member

@mattdowle mattdowle left a comment

Choose a reason for hiding this comment

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

Great find and nice fix!

@mattdowle mattdowle merged commit d70cfac into master Jul 8, 2017
@mattdowle mattdowle deleted the fread2260 branch July 8, 2017 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition causes fread to read the data incorrectly occasionally

3 participants