MT hardening FileInfoReader #568 #569
Conversation
The value of the barrier (true/false) was never read. It was only checked for !=null. The value had been read in waitOnSelf() without any multithreading semantics like synchronize/volatile. Instead now we use a thread safe AtomicBoolean. The synchronization blocks are still used for wait()/notifyAll() eclipse-equinox#568
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
| */ | ||
| private void waitOnSelf() { | ||
| schedule(); | ||
| while (barrier[0] == null) { | ||
| while (!barrierReached.get()) { |
There was a problem hiding this comment.
I must confess that I can't get what is the purpose of this loop... if I schedule a job once how can I join it more than once?
@merks any clue whats the purpose of this?
|
CountDownLatch can not be reset. CyclicBarrier can be reset but not read/mixed with the wait loop that askes for cancel. If you find any better solution please do. Meanwhile this could improve the situation. |
| synchronized (barrier) { | ||
| while (barrier[0] == null) { | ||
| synchronized (barrierReached) { | ||
| while (!barrierReached.get()) { |
There was a problem hiding this comment.
This changes behavior before any value TRUE/FALSE would have ended the loop now it loops forever if set to false.
The true/false value is actually never evaluated in the code of question, the only purpose seem to be to know if it has ended "normal" or for "unkown" reasons (maybe debugging?) |
Please add a testcase to prove it
from a code-review point of view this changes to much here to make it save. e.g. my proposed absolute minimal change (so it has a chance to get into RC phase) would be to refactor the check in the while into a method that synchronize on the array. That's not nice but if it is a thread-visibility problem would fix the problem in a way that can confidently be reviewed. |
|
We're in the RC2 phase so nothing will get done for this release. I'm really uncomfortable changing something here simply because we theorize that it might be broken with no reproducible way to test that we've actually fixed what we think is the problem rather than simply changed the behavior a bit and hoping for the best that this somehow helps something. How many times has even the most simple, innocent change broken something? I've lost count. But that's just my opinion and perhaps it's simply paranoia... |
|
I don't want this for any RC but M1. Simply adding a synchronize around an endless loop would harm more then help: The only use of synchronize in that class is the notify/wait mechanism - that does not exist in waitOnSelf(). |
I didn't suggested to put it around the loop ...
Then please fix up the PR/Commit message this PR seem to do something different then. Maybe it would be better to have two PRs for two things as well. |
@laeubi please provide such. |
The value of the barrier (true/false) was never read. It was only checked for !=null. The value had been read in waitOnSelf() without any multithreading semantics like synchronize/volatile. Instead now we use a thread safe AtomicBoolean. The synchronization blocks are still used for wait()/notifyAll()
#568