Eliminate benign race condition in DirectStreamObserver and make first and subsequent iterations consistent #33419
Conversation
A dynamic race condition detector correct identifies an unsynchronized read-after-write between the field initializer and the first call of onNext. Additionally, this fixes a benign inconsistency in which it would wait one extra message the first time around.
|
R: @scwhittle @t2h6 |
kennknowles
changed the title
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
It did trip the unit tests with an off-by-1. I don't yet know if it is truly incorrect (likely) or the unit tests were synced with the code but the actual value is arbitrary. |
sdks/java/core/src/main/java/org/apache/beam/sdk/fn/stream/DirectStreamObserver.java
Show resolved
Hide resolved
| @@ -69,7 +69,7 @@ public DirectStreamObserver(Phaser phaser, CallStreamObserver<T> outboundObserve | |||
| @Override | |||
| public void onNext(T value) { | |||
| synchronized (lock) { | |||
| if (++numMessages >= maxMessagesBeforeCheck) { | |||
| if (++numMessages > maxMessagesBeforeCheck) { | |||
There was a problem hiding this comment.
it seems that with the -1 -> 0 and the >= we still have the same behavior for the first triggered check. But subsequent triggered checks would take 1 more than previous incorrect behavior.
For example if maxMessagesPerCheck was 1, before we would check on
2nd message
and then every message afterwards. But now we would correctly check every other message. So if the test fails it seems like the test was verifying the bad behavior.
A dynamic race condition detector correct identifies an unsynchronized read-after-write between the field initializer and the first call of onNext.
Additionally, this fixes a benign inconsistency in which it would wait one extra message the first time around.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.