#1412 fix concurrency issue in CopyOnWriteMap#1419
Conversation
PR Summary
|
asolntsev
force-pushed
the
fix/1412-flaky-error
branch
from
938f3a2 to
4c4656a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1419 +/- ##
============================================
+ Coverage 92.22% 92.24% +0.02%
Complexity 3165 3165
============================================
Files 320 320
Lines 6173 6177 +4
Branches 592 592
============================================
+ Hits 5693 5698 +5
Misses 335 335
+ Partials 145 144 -1 ☔ View full report in Codecov by Sentry. |
asolntsev
force-pushed
the
fix/1412-flaky-error
branch
from
9242112 to
8ab0af1
Compare
asolntsev
changed the title
CopyOnWriteMap
kingthorin
left a comment
kingthorin
left a comment
There was a problem hiding this comment.
Seems reasonable to me.
| } catch (RuntimeException expected) { | ||
| assertThat(expected).hasMessageStartingWith("County is not configured for postcode " + zipCode); | ||
| } catch (RuntimeException | Error e) { | ||
| if (!e.getMessage().startsWith("County is not configured for postcode ")) { |
There was a problem hiding this comment.
Are we sure there couldn't be exception/error with null message?
There was a problem hiding this comment.
Good point. Yes, theoretically it can happen.
When any unexpected error happened in Issue759Test#WorkerThread, it was not logged anywhere, just disappeared. _countDownLatch was not decreased, and the test failed with "Test did not complete within 12 second" error.
the default method `Map.getOrDefault` is not thread-safe: the condition `if (v == null && containsKey(key))` leaves `v = null` if the `key` was added by another thread exactly at the same moment. Then both conditions are true: `v==null` and `containsKey(key)`. Now `CopyOnWriteMap.getOrDefault` does NOT check `containsKey(key)`. As a consequence, `CopyOnWriteMap` doesn't allow putting `null` values to the map.
asolntsev
force-pushed
the
fix/1412-flaky-error
branch
from
8ab0af1 to
0babf2f
Compare
|
@kingthorin I think we should release DF 2.4.2 |
|
@asolntsev I think we should perhaps first have the documentation for DF 2.4.1 before releasing 2.4.2? |
|
@bodiam To be honest, I know nothing about documentation for DF 2.4.1 (and all other versions) :) |
|
@asolntsev I know, I know. I'll organize a training session soon ;) But maybe I should make a quick document with steps involved, I know it's not the most exciting thing to work on, but I think it's kinda nice when the documentation is a little up to date. |
|
@kingthorin @bodiam Hi. What about releasing DataFaker 2.4.2? |
|
I'm keen. @kingthorin ? |
|
I just merged #1438 , happy to give that one a shot. |
|
Ya if we can get that roll PR in first that'd be handy |
|
Perhaps it would be nice to get this one in (#1414), or we keep this for 2.5? |
|
Oops sorry I missed your comment about 1414 earlier somehow. I just came in here to say I'd done the 2.4.2 release. |
|
Thanks for that. It doesn't look the like version in the pom.xml got updated though? |
|
Oh, it creates a PR. Oki, that works! |
Sorry, but README is still not updated. :) 
|
|
Yup that hasn't been automated yet. |
4 participants
The default method
Map.getOrDefaultis not thread-safe: the conditionif (v == null && containsKey(key))leavesv = nullif thekeywas added by another thread exactly at the same moment. Then both conditions are true:v==nullandcontainsKey(key).Now
CopyOnWriteMap.getOrDefaultdoes NOT checkcontainsKey(key).P.S. As a side-effect,
CopyOnWriteMapdoesn't allow puttingnullvalues to the map. But we don't need it anyway.