rxjs icon indicating copy to clipboard operation
rxjs copied to clipboard

Tap kills the Subject on unsubscription

Open voliva opened this issue 3 years ago • 2 comments

Describe the bug

When using tap passing in a Subject directly (i.e. without using { next, error, complete }), if the resulting observer creates a subscription and unsubscribes from it, then the Subject is also unsubscribed: If a new subscriber comes, it will throw an ObjectUnsubscribedError.

I've bisected this happened somewhere in between 7.2.0 and 7.3.0

Expected behavior

When the resulting subscription is closed, the Subject should not be killed: The new subscription should work just as fine as the initial one.

Reproduction code

const subject = new Subject();

interval(1000).pipe(
  tap(subject),
  take(2),
  repeat(2)
).subscribe(v => console.log(v))

Reproduction URL

https://stackblitz.com/edit/rxjs-a9l4j2?file=index.ts

Version

7.3.0

Environment

No response

Additional context

No response

voliva avatar Oct 03 '22 09:10 voliva

I see this was introduced in #6527

voliva avatar Oct 03 '22 09:10 voliva

The API change on tap was quite substantial when used with Subjects, and I think it was a valid use case. tap accepted Observer, and a Subject is an Observer - sometimes it was used to replicate a stream for different reasons (e.g. circular references)

Now not only tap will kill the Subject by calling unsubscribe() on it when one of the underlying subscription closes, but it will also create lingering subscriptions every time someone subscribes to the resulting observable, because from that PR, tap also now calls .subscribe() on the subject when someone subscribes.

I'm not really sure what would be the best solution for this now. Ideally that change should've happened in a major release (8.x), or maybe the functions that it would call from observers should've been in past tense (.subscribed(), .unsubscribed(), .finalized()) instead. Another option would be to make special cases for Subjects, but I'm not sure how would this work for all of them.

voliva avatar Oct 05 '22 08:10 voliva