-
Notifications
You must be signed in to change notification settings - Fork 894
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
[dlog] Issue 1544: ConcurrentModificationException with nonblocking logReader.readNext(true) #1558
Conversation
…r.readNext(true) *Motivation* Fixes apache#1544. ConcurrentModificationException is thrown when trying to access a non-thread-safe hashmap from different threads. *Changes* Make sure accessing to this non-thread-safe hashmap is under synchronized block. *Tests* It is a bit tricky to reproduce this race condition in a unit test or an integration test. so not going to attempt adding any tests.
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.
I wonder why spotbugs is okay with this change. We are not always accessing listeners using a synchronized block.
By the way, I don't this is it good to synchronize over a ConcurrentMap, maybe an alternative solution is to clone the keySet and iterate over the clone.
Maybe I am missing something
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.
Sorry now I see, listeners is not the main listeners field.
+1
retest this please |
rebuild java8 |
rebuild java9 |
…logReader.readNext(true) Descriptions of the changes in this PR: *Motivation* Fixes #1544. ConcurrentModificationException is thrown when trying to access a non-thread-safe hashmap from different threads. *Changes* Make sure accessing to this non-thread-safe hashmap is under synchronized block. *Tests* It is a bit tricky to reproduce this race condition in a unit test or an integration test. so not going to attempt adding any tests. Master Issue: #1544 Author: Sijie Guo <sijie@apache.org> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None> This closes #1558 from sijie/issue_1544, closes #1544 (cherry picked from commit ec7fb62) Signed-off-by: Sijie Guo <sijie@apache.org>
Descriptions of the changes in this PR:
Motivation
Fixes #1544. ConcurrentModificationException is thrown when trying to access a non-thread-safe hashmap from different threads.
Changes
Make sure accessing to this non-thread-safe hashmap is under synchronized block.
Tests
It is a bit tricky to reproduce this race condition in a unit test or an integration test. so not going to attempt adding any tests.
Master Issue: #1544