fix(bufferTime): do not create buffer when unsubscribed #1949
Conversation
kwonoj
force-pushed
the
fix-buffertime-emptyinterval
branch
from
3730663
to
f67d710
Compare
| @@ -120,7 +120,7 @@ class BufferTimeSubscriber<T> extends Subscriber<T> { | |||
| private scheduler: Scheduler) { | |||
| super(destination); | |||
| const context = this.openContext(); | |||
| this.timespanOnly = bufferCreationInterval == null || bufferCreationInterval < 0; | |||
| this.timespanOnly = bufferCreationInterval === null || bufferCreationInterval < 0; | |||
There was a problem hiding this comment.
Help me understand plz, why is there a notable difference between undefined and null here? i.e. why the fact that == null would match undefined too is a problem?
There was a problem hiding this comment.
wow, it's my bad. apologize. This isn't legit complete fix and code should be updated more.
There was a problem hiding this comment.
Phew! I was racking my brain!! 😅😅
kwonoj
force-pushed
the
fix-buffertime-emptyinterval
branch
2 times, most recently
from
9a046ae
to
bc8be00
Compare
| this.closeContext(context); | ||
| const closeAction = context.closeAction; | ||
| closeAction.unsubscribe(); | ||
| this.remove(closeAction); | ||
|
|
||
| if (this.timespanOnly) { | ||
| if (this.timespanOnly && this.contexts !== null) { |
There was a problem hiding this comment.
this is actual changes, do not attempt to open buffer if it's already unsubscribed.
There was a problem hiding this comment.
Can we get a regression test around this?
There was a problem hiding this comment.
Not sure. This occurs with async context, not able to think trivial way to create test cases at the moment writing PR. Let me think a bit more.
There was a problem hiding this comment.
I thought this bit, but wasn't able to come up with clean way to create test coverage via test scheduler (or synchronous way) to mimic unsubscription occurs before scheduled action triggered.
kwonoj
force-pushed
the
fix-buffertime-emptyinterval
branch
from
bc8be00
to
59b149c
Compare
kwonoj
changed the title
1 similar comment
|
I think PR #1998 covers same issue and also having test coverage backing it, closing this PR without merging. |
Observing functions (callbacks) are given to `.subscribe(next, error, complete)` by an outside code. But `rxjs` dictates a context of execution, instead of leaving it `undefined`. It sort of disrespects separation between callbacks that come from outside, and its own internals on the public API of the library. As a result, there are issues ReactiveX#3229, ReactiveX#1981, ReactiveX#1949, ReactiveX#2140, that happen due to `rxjs` mangling with execution context. Library behaves in an unexpected way. This change fixes an unsanctioned introduction of context to callback. Now, it is possible, that this weird behaviour was introduced to help some other parts of the lib. Tests should show this. And may be those places should change instead of keeping users of the library puzzled.
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description:
This PR fixes checking if given
bufferCreationIntervalisnullor not, to execute legit paths when only timespan's supplied.Related issue (if exists):