-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
Let's just take out a lock - and write a test that covers it. It's more obviously correct.
Generate changelog in
|
future.complete(delegate.get()); | ||
} catch (Throwable t) { | ||
future.completeExceptionally(t); | ||
synchronized (lock) { |
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.
this is an uncontended lock except in the case of the bug.
e96a501
to
d9ae980
Compare
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.
Change is correct, but should have a changelog explaining how the old one would fail. Thanks!
Whoops, I need to validate the SettableFuture part that just came out
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.
Implementation looks good, and the test is legit!
return counter.incrementAndGet(); | ||
}); | ||
List<ListenableFuture<?>> futures = IntStream.range(0, poolSize) | ||
.mapToObj(index -> executorService.submit(() -> assertIncreasing(supplier))) |
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.
This is neat!
Released 0.185.3 |
Let's just take out a lock - and write a test that covers it. It's more
obviously correct.