fix(audit): mirror source if durations are Observable.empty() #2595
Conversation
src/operator/audit.ts
Outdated
| @@ -86,6 +86,9 @@ class AuditSubscriber<T, R> extends OuterSubscriber<T, R> { | |||
| this.destination.error(errorObject.e); | |||
| } else { | |||
| this.add(this.throttled = subscribeToResult(this, duration)); | |||
There was a problem hiding this comment.
shouldn't synchronized subscription to be added into subscription as well?
There was a problem hiding this comment.
I don't understand what you mean. Can you explain?
There was a problem hiding this comment.
Refer this one - https://github.com/ReactiveX/rxjs/pull/1490/files#diff-4c9df56dc210f37759b96afb96e5fdd0R135
if subscribeToResult is synchronously subscribed, returned subscription is immediately unsubscribed already, so there's no meaning to add subscription for managing it (this.add). So in that case, check returned subscription and add only if it's asynchronous would be better.
There was a problem hiding this comment.
I've updated the PR to address this.
src/operator/audit.ts
Outdated
| @@ -86,6 +86,9 @@ class AuditSubscriber<T, R> extends OuterSubscriber<T, R> { | |||
| this.destination.error(errorObject.e); | |||
| } else { | |||
| this.add(this.throttled = subscribeToResult(this, duration)); | |||
| if (this.throttled.closed) { | |||
There was a problem hiding this comment.
I'm curious about this change. Wouldn't this make it so a completed Subject would cause this behavior? That seems incorrect.
Is this really a subscription ordering thing? What do you think @trxcllnt?
There was a problem hiding this comment.
I have assumed this change's similar to this -> #1490, where there are synchronous observables are supplied subscribeToResult completes immediately instead of scheduled.
There was a problem hiding this comment.
When I ran this code under a debugger, that's exactly what happened: inside subscribeToResult, the observable completed, so by the time it is returned, it is already completed, so we shouldn't throttle subsequent results anymore.
I have two questions:
- How should we solve this problem?
- How do we avoid this problem elsewhere as the cold('|') observable used in tests doesn't complete immediately.
|
Who can merge this? |
|
I can, but checking with release cadences to make right version. Guess this is patch? /cc @benlesh |
|
There's a problem with this commit, when if (!innerSubscription || innerSubscription.closed) { |
…ion Observable completes Fixed the problem when subscribeToResult() returns null it will crash described by @deadbeef84. audit() crashs with Observable.of()
|
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:
The
auditoperator was suppressing all events if the duration wasObservable.empty()or any observable that was already complete.There was already a similar test for this, but it relied upon
cold('|'). It might be worth considering either duplicating tests involvingcold('|')to useObservable.empty()or special-casing it to returnObservable.empty()