kennknowles commented on code in PR #33419:
URL: https://github.com/apache/beam/pull/33419#discussion_r1910568990
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/fn/stream/DirectStreamObserver.java:
##########
@@ -53,7 +53,7 @@ public final class DirectStreamObserver<T> implements
StreamObserver<T> {
private final int maxMessagesBeforeCheck;
private final Object lock = new Object();
- private int numMessages = -1;
Review Comment:
> can this be annotated with GuardedBy?
I don't _think_ it would help this case, since it was a dynamic analysis
that only records race conditions that actually happen. I will add the
annotation of `@GuardedBy(lock)` and that might inform static analyses.
We could actually guard the initialization by doing `synchronized(lock) {
numMessages = -1 }` in the constructor but I still don't see why it would make
sense. Incidentally I also don't know why the lock isn't `this` since they are
one-to-one.
> This seems ok, but I'm not clear why this is less racy? Why is
initializing to 0 different from initializing to -1?
Yea it is a bit weird. It only makes sense to me if the setting to -1 is
somehow compiled to be after the constructor and subject to reordering. It
seems like it could be a bug in the race detector not realizing the object is
still under construction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
