feat(share): use another observable to control resets

[backbone87]

This is a PoC. I will add tests, if the approach is agreed up on.

Description:
Extends the share operator to allow passing an observable factory to control the reset behavior and enable reset delays.

declare function createObservableWithExpensiveInitialization(): Observable<number>;

const number$ = createObservableWithExpensiveInitialization().pipe(share({ resetOnRefCountZero: () => timer(5000) }));
concat(number$.take(1), number$.take(1)).subscribe(); // this wont unsubscribe (unless your process is REALLY busy, one could say "stuck")

Related issue (if exists):
closes #4033

[backbone87]

i have a modified version, which passes the existing test cases (and is more concise), while having the exact same public API.

the current approach resulted from a previous attempt, where i used mergeAll insteadof switchAll, to allow "concurrently" running resets. after putting some thought into it, i came to the conclusion that this is not needed (and would involve much more complicated logic beyond just using mergeMap), because the error/complete reset notifiers should always overrule/replace the refCount reset notifiers.

[backbone87]

what can i do, to push this forward?

[cartant]

what can i do, to push this forward?

Nothing. ATM, I have higher v7 priorities. This would not be a breaking change, so we can implement this after v7 is done/released. It's not a v7 blocker, so it's not near the top of my things-to-do list.

[backbone87]

i actually have the need for this in one of my projects. it is not critical, but would improve behavior. i tried to find a work around by combining other operators (v6), but failed to achieve this behavior, except for having a permanent subscription so it never resets.

[cartant]

i actually have the need for this in one of my projects.

Unfortunately, that doesn't change my priorities.

[cartant]

In general, I think this is okay, but it's going to need a whole bunch of tests. And it's going to need to be updated to account for the change in #6151 - presumably, once it's no longer a draft, GitHub will show that there's a conflict that needs to be resolved. Maybe start with the latter.

[backbone87]

• rebased onto latest master
• added docs
• switched to lazy options normalization

[cartant]

This PR is going to need to be rebased again, to account for the change introduced in #6370

Also, I'd prefer this call to cancelReset to be removed. If there's a situation that cannot happen, that should be expressed in the tests and not by calling something that should always be ineffectual:

rxjs/src/internal/operators/share.ts

Lines 207 to 210 in f32f79e

	if (refCount === 0 && !hasErrored && !hasCompleted) {
	cancelReset(); // paranoia, there should never be a resetConnection, if we reached this point
	resetConnection = handleReset(resetAndUnsubscribe, resetOnRefCountZero);
	}

After you've done the rebase, I'll review this and will let you know what needs to be tested. If you're unfamiliar with marble tests, I'll write one to show you what we need to do. This is going to need a lot of tests.

[backbone87]

• rebased against master
• removed paranoia reset
• added general documentation about reset behavior

edit:
I have used marble testing before, and will try to implement the tests based on your requirements.
I think mainly the synchronous version of notifier factory resets need extensive testing (so it behaves like when you pass a boolen).

[cartant]

I've read through this and I have no further suggestions. It LGTM. I'll prepare a list of what I think we need to test and I'll post it here sometime tomorrow.

[backbone87]

i have added some tests already

• execute all existing tests with equivalent sync reset notifiers
• tests for async/deferred reset on ref count zero notifiers

[cartant]

IDK that I'm going to get around to creating that list, today. But one of the things we definitely need to test is that when a reset due to an error or a complete is initiated, any factory passed via the resetOnRefCountZero is not called.

[backbone87]

• more sync reset notifier equivalents:
  • NEVER after notify for resetting tests
  • error after notify for resetting tests
  • NEVER for non-resetting tests
  • error for non-resetting tests -> these fail the tests - different failures for the same tests depending on useDeprecatedSynchronousErrorHandling -> what should the behavior be?
• added "should not reset on refCount 0 after reset notifier errored?" -> fails -> what should the behavior be?

also, I may have the fear that under some circumstances NEVER notifiers may leak memory. i used jest-leak-detector for such tests in the past, how is this tested in rxjs with mocha? I rechecked this and the suspected source of leak is covered somewhat: I feared dangling notifiers when the source completes/errors while no one is subscribed, but this cancels the existing resetOnRefCountZero notifiers and starts the resetOnError/Complete notifiers. the latter ones then are truly sticking around forever (if never and not notifying), but this is expected. In general, i would say it is an anti-pattern to use unconditional NEVERs as notifiers (in any operator).

[backbone87]

added some cleanups to the impl:

• optimize internal naming
• remove unnecessary typings
• avoid non-null type assertation

[cartant]

useDeprecatedSynchronousErrorHandling

I would not add tests for this. It's going to be removed.

Regarding the handling of errors from notifiers, IMO we should make no attempt to handle and report them. There is really no avenue for their being reported anyway. Leave them fall through to config.onUnhandledError - which you can use in the tests.

rxjs/src/internal/config.ts

Lines 22 to 31 in c23e567

	/**
	* A registration point for unhandled errors from RxJS. These are errors that
	* cannot were not handled by consuming code in the usual subscription path. For
	* example, if you have this configured, and you subscribe to an observable without
	* providing an error handler, errors from that subscription will end up here. This
	* will _always_ be called asynchronously on another job in the runtime. This is because
	* we do not want errors thrown in this user-configured handler to interfere with the
	* behavior of the library.
	*/
	onUnhandledError: ((err: any) => void) | null;

should not reset on refCount 0 after reset notifier errored?

It should not reset. The only signal for any of the resets is a next notification. If one isn't received, there should be no reset. Errors should be reported via config.onUnhandledError as above.

In general, i would say it is an anti-pattern to use unconditional NEVERs as notifiers (in any operator).

Yeah, but we still need to test for it. This is done with other operators, IIRC. It will leak, but that's expected.

[backbone87]

• should contain all necessary tests now, if something is missing let me know
• squashed commits

[backbone87]

in regards to notifiers emitting errors: we could attach a noop error handler when subscribing to notifiers, if the user wants to handle/react to errors himself he could use tap or catchError

[cartant]

in regards to notifiers emitting errors: we could attach a noop error handler ...

I think it should be kept as simple as possible. The user is already providing a factory that returns an observable. If a potential error is something they thing they can handle, they already have the opportunity to compose that into the observable they are giving us.

[benlesh]

The list I followed was more of a matrix. You pick all of the ones that make sense from this, for each observable involved:

	sync	async	mixed
single value
multi value
empty
never
error
values and error
values and never

(clearly, by these standards, none of our operators has 100% coverage, probably, but like I said, pick the ones that make the most sense.)

[backbone87]

i dont fully understand your table. but i guess you mean test coverage based on notifier characteristics? basically, the handling of the notifier observable is tied to take(1) semantics, but I try to answer:

	sync	async	mixed	should reset?
single value	x	x		yes
multi value	same as value + error			yes
empty	x	same as sync		no
never	x	same as sync		no
error		x		no
value + error	x			yes
value + never	x			yes

(what do you mean with mixed? a sync emit + something async after?)

[backbone87]

added some more test cases around sync multi value notifier, as well as firehose observable + sync notifier and explicit test for sync resubscribe with sync notifier.

[cartant]

This is great. I'm more than happy with the tests. Thanks, @backbone87. I've suggested adding a comment to your dest declaration refactor, but approving this now. Thanks.

[benlesh]

Okay. I finally had time to review this again.

This looks really good. The only thing I'm not sure about is that we need to delay reconnect for complete and error. While I think it "rounds out" the API, I'm not sure that I can think of a use case for that.

In the case of complete and error the source is signaling that it's done and can no longer send anything else. What is the purpose of delaying the reseting of the connector at that point? The source can't possibly send anything else. Delaying isn't going to do anything.

I can DEFINITELY think of use cases for delaying the reset on ref count. I can think of plenty of use cases there, because that's something downstream. It's not the source saying it can't send anything else.

@cartant, since you approved this, maybe you have other thoughts, but it seems like we should only be supporting this for resetOnRefCountZero. I think that's tremendously useful. But in the other cases, if we enable it, we'll end up having to support people who use it that way. I'm concerned that those usages will be really weird, possibly wrong-headed, and we'll be on the hook for supporting them.

Otherwise, I generally approve of this entire PR. I just think we want to ratchet it back to only the resetOnRefCountZero option.

[benlesh]

After some discussion with @cartant, the feature makes sense. He pointed out that it could be used to cache a completed source for a specified amount of time. That's a reasonable use case for completion. Still unsure about error, however, at the point that two of them accept it, it's probably confusing if they all don't accept it.

[benlesh]

Thank you, @backbone87, for this great contribution!