[tryout fix] concurrency bug in NamespacedHierarchicalStore#computeIfAbsent(Object, Object, Function) #5171 #5209
#5213
Conversation
✅ All tests passed ✅🏷️ Commit: 8287bea Learn more about TestLens at testlens.app. |
...ngine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java
Outdated
Show resolved
Hide resolved
| @SuppressWarnings("ReferenceEquality") | ||
| @API(status = MAINTAINED, since = "6.0") | ||
| public <K, V> Object computeIfAbsent(N namespace, K key, Function<? super K, ? extends V> defaultCreator) { | ||
| Preconditions.notNull(defaultCreator, "defaultCreator must not be null"); |
There was a problem hiding this comment.
The precondition itself enforces (for good reason, I admit) a specific string pattern, which is beneficial to avoid being overly vague.
Still, the convention principle seems to always win when using the good old POJO approach.
Considering this kind of implementation detail, and in the sake of not making things too DRY, there is some of the same reasoning as correctly mentioned by @mpkorstanje about not introducing too much coupling and extra, extra.
Of course, this is kind of debatable, non-priority, and off-topic—just wanted to give some reasoning.
In: #5209 (comment)
There was a problem hiding this comment.
In isolation the reasoning makes sense. But notNull is but one precondition of many. There are also notBlank, containsNoNullElements, ect. So we use Precondition for consistency.
| class ConcurrencyIssue5171 { | ||
|
|
||
| @Test | ||
| void computeIfAbsentDoesNotDeadlockWithCollidingKeys() throws Exception { |
There was a problem hiding this comment.
actually not sure, if this is any good... this topic too much for my 2ct., cannot afford.
Just curious as it was failing the current impl., trying to fix it right away lead to this PR.
Maybe you can check it out. Thanks.
.../src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java
Outdated
Show resolved
Hide resolved
Pankraz76
force-pushed
the
fix-TestWatcher
branch
2 times, most recently
from
9509328 to
cb64820
Compare
Pankraz76
force-pushed
the
fix-TestWatcher
branch
from
cb64820 to
1758e82
Compare
Pankraz76
force-pushed
the
fix-TestWatcher
branch
2 times, most recently
from
a995123 to
673e831
Compare
…fAbsent(Object, Object, Function)` junit-team#5171 junit-team#5209 Signed-off-by: Vincent Potucek <[email protected]>
Pankraz76
force-pushed
the
fix-TestWatcher
branch
from
673e831 to
8287bea
Compare
|
kindly asking, if this is any good? Thanks to the community. |
There was a problem hiding this comment.
Thanks for offering!
kindly asking, if this is any good?
It's not bad. If I was working on this bit of code I would pick up the var additions, this. removal, and removal of local variables. The tests look like they could be useful too.
But there are other things to consider as well.
Changing code just to make it look prettier provides only a small benefit but also comes with a cost. It tends to make the git blame harder to read, makes PRs bigger than they need to be, and overall takes up maintainer time in review. The latter is especially scarce.
Right now for #5171, we haven't quite figured out yet what we want the solution to be, nor do we have a reliable reproducer that we can put into a unit test. So making a PR is rather premature. But I do appreciate the gesture and I'll keep the ideas behind your changes in mind.
PS: If you can reliably reproduce the effects of #5171 as a unit test against the NamespacedHierarchicalStore, I think the most appropriate way to provide that would be as a comment on #5171.
| @SuppressWarnings("ReferenceEquality") | ||
| @API(status = MAINTAINED, since = "6.0") | ||
| public <K, V> Object computeIfAbsent(N namespace, K key, Function<? super K, ? extends V> defaultCreator) { | ||
| Preconditions.notNull(defaultCreator, "defaultCreator must not be null"); |
There was a problem hiding this comment.
In isolation the reasoning makes sense. But notNull is but one precondition of many. There are also notBlank, containsNoNullElements, ect. So we use Precondition for consistency.
2 participants
[tryout fix] concurrency bug in
NamespacedHierarchicalStore#computeIfAbsent(Object, Object, Function)#5171 #5209I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@APIannotationscoverage:
before:
after: