Synchronizer: concurrency fixes #147
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
champtar
force-pushed
the
SynchronizerImpl
branch
from
13ae337
to
ca2d708
Compare
HashMap are not thread safe, even when reading, so we can't use them outside of synchronized blocks, else we can have some random crash or data/result inconsistencies between threads. ConcurrentHashMap is a well optimised beast, so use it. Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
If you synchronize for writing you have to synchronize for reading too. Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
champtar
force-pushed
the
SynchronizerImpl
branch
from
ca2d708
to
d546b37
Compare
|
The first commit is fine. In the second one, the unsynchronized that were there before reads are okay because the conditions are checked again after obtaining a lock. The valid changes in the second commit are thus only the parameter validations. |
|
This type of check is only ok if you have an initial state (set in the constructor) and you never go back to it, and that's not the case here (see removeMapping). |
|
@bgrozev informed me that this code isn't even in use. As I'm lacking time to go through it again, I'm closing this PR. |
|
@ibauersachs this was spotted with FindBugs and haven't checked that it's used |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Except if you use Atomic* or volatile, you have to lock/synchronize for both writing and reading.