-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enforces ordering on concurrent subscription to FluxRefCount
#3517
Conversation
Signed-off-by: OlegDokuka <odokuka@vmware.com>
Late subscribe may observe zero events when multiple inners subscribes to FluxRefCount simultaneously so the one which does not invoke connect method subscribes to the FluxPublish too late (e.g. the one which call connect subscribe faster and invoke connect method earlier) ``` subscribe 1 subscriber 2 | | count + 1 (1) | | count + 1 (2) | | | subscribe | | | connect | onComplete subscribe (hanging) ``` to solve the problem subscribe should happen before count is incremented so we have strong ordering Signed-off-by: OlegDokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
FluxRefCount
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.
Perhaps adding corresponding cases for Flux.error(Throwable)
in the fusion/hide/delay variants to also validate how error is handled could be worthwhile?
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.
fixed
|
||
RefCountInner(CoreSubscriber<? super T> actual, RefCountMonitor<T> connection) { | ||
volatile int state; //used to guard against doubly terminating subscribers (e.g. double cancel) |
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.
I suppose the comment is no longer relevant, as the state represents more situations and before it was just about termination/cancellation, while now it also represents the readiness (monitor set). Perhaps we can remove it.
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.
fixed
@OlegDokuka , if you wish to know whether the tests I wrote pass with your fix, I can confirm they do! :) Thanks! |
Signed-off-by: OlegDokuka <odokuka@vmware.com>
@OlegDokuka this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to |
Signed-off-by: OlegDokuka <odokuka@vmware.com>
Signed-off-by: OlegDokuka <odokuka@vmware.com>
Late subscribe may observe zero events when multiple inners subscribe to
FluxRefCount
simultaneously so the one which does not invoke theconnect
method subscribes to theFluxPublish
too late (e.g. the one which calls theconnect
subscribe faster and invoke theconnect
method earlier than the others):to solve the problem subscribe should happen before the count is incremented so we have strong ordering guaranteed by the
synchronize
blockfixes #3487