-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Behavior subject time gap fix 2 #1185
Behavior subject time gap fix 2 #1185
Conversation
RxJava-pull-requests #1095 FAILURE |
Wow this is quite the change :-) I need fresher eyes on this one so will come back to it. Thank you very much for tackling this! |
return new PublishSubject<T>(onSubscribe, subscriptionManager, lastNotification); | ||
@Override | ||
public void call(SubjectObserver<T> o) { | ||
o.emitFirst(state.get()); |
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.
Why is PublishSubject
emitting the last received value onAdd? A PublishSubject
should only receive values going forward, nothing historical.
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.
@akarnokd Can you help me understand this one?
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 helps eliminate race and sequence gap between adding a subscriber and terminating concurrently without the use of CountDownLatch.
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.
At least for BehaviorSubject. I can't remember why I left it there; maybe it is completely unnecessary and onAdded may be no-op. Either the add succeeds and the subject becomes reachable by onCompleted or fails to add and onTerminated is invoked. In the first case, state-value is null and first won't emit anything. I'll run some tests with nop tomorrow.
This is non-trivial code, so I may have missed something, but this looks like excellent forward progress on what we had to add the functionality needed for Very nice work. |
Behavior subject time gap fix 2
Based on discussions in #1181, I've unified the behavior among the Subjects.
BehaviorSubject doesn't seem to exhibit issue #658 and #1184.
A second pair of eyes would be great to confirm the correctness and check if the removal of the CountDownLatch was the correct approach.