BUG: Address race condition

[blowekamp]

Addresses issue #4711.

When RelabelConnectedRegion is called by ThreadedConnectivity, both outputLabel and requeredLabel are the same value. Don't re-write the same value.

PR Checklist

• [ ] No API changes were made (or the changes have been approved)
• [ ] No major design changes were made (or the changes have been approved)
• [ ] Added test (or behavior not changed)
• [ ] Updated API documentation (or API not changed)
• [ ] Added license to new files (if any)
• [ ] Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
• [ ] Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for further development details if necessary.

[seanm]

@blowekamp cool, thanks! Do you need me to try with TSan or did you already?

[blowekamp]

@blowekamp cool, thanks! Do you need me to try with TSan or did you already?

Yes, please test to confirm. I have not configured TSan yet.

[seanm]

Yes, please test to confirm.

I'm afraid all 3 tests still fail under TSan.

I have not configured TSan yet.

Which platform/compiler do you use? TSan is quite easy to use with clang on unix-like OS (and with gcc AFAIK).

[blowekamp]

Well that is disappointing.

I am on OSX 12 ARM. Do you have any documentation for using this tool?

[seanm]

Very easy then, just add -fsanitize=thread to CMAKE_CXX_FLAGS and CMAKE_C_FLAGS.

For details: https://clang.llvm.org/docs/ThreadSanitizer.html

[blowekamp]

Very easy then, just add -fsanitize=thread to CMAKE_CXX_FLAGS and CMAKE_C_FLAGS.

For details: https://clang.llvm.org/docs/ThreadSanitizer.html

Yes, that was easy. The SLIC tests now pass local with the thread sanitizer flag enabled now.

[seanm]

Yes, that was easy. The SLIC tests now pass local with the thread sanitizer flag enabled now.

Sweet! Thanks. The change looks reasonable looking to me, but I don't know this code at all.

[dzenanz]

The change is small, but non-trivial. I can't dive now into the code to understand what it does, and to verify the change is OK.

[blowekamp]

@thewtex Looks like this PR was directly merged into release-5.4. Not sure what need to occur now, to get the release and release-5.4 branches is order.

[dzenanz]

Merge release-5.4 to release, then merge release into master?

[thewtex]

@thewtex Looks like this PR was directly merged into release-5.4. Not sure what need to occur now, to get the release and release-5.4 branches is order.

I went ahead and updated release and master -- happy to do so with an @ ping. Docs are also available here: https://docs.itk.org/en/latest/contributing/index.html#merge-a-topic

[blowekamp]

@thewtex Looks like this PR was directly merged into release-5.4. Not sure what need to occur now, to get the release and release-5.4 branches is order.

I went ahead and updated release and master -- happy to do so with an @ ping. Docs are also available here: https://docs.itk.org/en/latest/contributing/index.html#merge-a-topic

Thanks for the doc links. It's not clear to me what currently is the difference between the release and the release-x.x branches, and how to determine is a "patch" should got to just release or to both.

[thewtex]

Thanks for the doc links. It's not clear to me what currently is the difference between the release and the release-x.x branches, and how to determine is a "patch" should got to just release or to both.

release is not version specific, which release-X.X is.

They always go into both.

[seanm]

I went ahead and updated release and master

Meaning this should be reflected in today's cdash, right?

SLICFixture.Blank2DImage is still failing under TSan on my bot: https://open.cdash.org/tests/1617156531

Was it passing locally for you @blowekamp ?

Perhaps you fixed one race, but not all of them?

[thewtex]

Meaning this should be reflected in today's cdash, right?

Yes