-
-
Notifications
You must be signed in to change notification settings - Fork 668
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
BUG: Address race condition with threaded copying input to output #4988
BUG: Address race condition with threaded copying input to output #4988
Conversation
Move copying input to output to before threaded methods, to ensure the operation has been completed before modifications.
After reviewing the algorithm again, the approach does not produce output that is consistent with standard morphological operators. |
Modules/Filtering/BinaryMathematicalMorphology/include/itkObjectMorphologyImageFilter.hxx
Show resolved
Hide resolved
Trying to understand what it's actually doing here: It fills the entire output buffer with either ITK/Modules/Filtering/BinaryMathematicalMorphology/include/itkObjectMorphologyImageFilter.hxx Lines 95 to 119 in 08bc946
|
There are a lot of things in this filter that I am not following why they are don't they way they are or if they are correct. If what you are suggesting is correct then why just wan't the input copied to the output? Or why doesn't the pseudo-morphological operation that the input image as input? Some one could be using the filter or it could just be wrong 🤷 This change just fixes the race condition. |
Should we then add a warning note to the documentation, saying that the behavior of this filter is a bit unclear? |
@blowekamp thanks for working on this! You may have indeed fixed this write-write race, but the test still fails under TSan, now with a read-write race:
|
@seanm That is a different test that the itkErodeObjectMorphologyImageFilterTest one. Not sue sure why this hasn't gotten any approval yet. |
Oh, I see the mistake. #4969 says
I'll go fix the #4969 text... |
@dzenanz Please review. This a minimal change to address the race condition without changing the logic, only moving the pixel copying from the threaded method to the before threaded method. This filter's existential crisis is not related to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The while (!oRegIter.IsAtEnd())
loop that you moved into the body of BeforeThreadedGenerateData() looks inefficient to me. I wonder, why not using itk::ImageAlgorithm::Copy
? Because it looks like it's just copying all pixels from input to output! It's also a pity that the task of copying pixels is no longer parallelized with your PR, because I believe it's an "embarrassingly parallelizable" task on its own.
I also find the way this filter makes use of "faces" rather suspicious, as I mentioned at #4969 (comment) But as you say "This a minimal change to address the race condition without changing the logic", and I like PRs that present a minimal change to address a problem. 😃 I do think it could solve the race condition, so I'll approve it anyway, as it is now. Thanks Bradley! 👍
Good as-is, would be better with Niels' suggestion. |
Move copying input to output to before threaded methods, to ensure the operation has been completed before modifications.
Addresses #4969
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.