Skip to content
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

feat(share): use another observable to control resets #6169

Merged
merged 1 commit into from
May 20, 2021

Conversation

backbone87
Copy link
Contributor

@backbone87 backbone87 commented Mar 23, 2021

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
Copy link
Contributor Author

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.

@benlesh benlesh requested a review from cartant March 24, 2021 20:27
@backbone87
Copy link
Contributor Author

what can i do, to push this forward?

@cartant
Copy link
Collaborator

cartant commented Apr 10, 2021

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
Copy link
Contributor Author

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
Copy link
Collaborator

cartant commented Apr 11, 2021

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

Unfortunately, that doesn't change my priorities.

@cartant
Copy link
Collaborator

cartant commented Apr 29, 2021

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.

@cartant cartant added the 7.x Issues and PRs for version 7.x label May 5, 2021
@backbone87 backbone87 force-pushed the feature/share-reset-notifier branch from d682c57 to 4d20d74 Compare May 5, 2021 18:41
@backbone87
Copy link
Contributor Author

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

@backbone87 backbone87 force-pushed the feature/share-reset-notifier branch from 4d20d74 to f32f79e Compare May 5, 2021 18:48
@cartant cartant mentioned this pull request May 5, 2021
40 tasks
@cartant
Copy link
Collaborator

cartant commented May 7, 2021

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:

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 backbone87 force-pushed the feature/share-reset-notifier branch from f32f79e to 1a93537 Compare May 7, 2021 10:33
@backbone87
Copy link
Contributor Author

backbone87 commented May 7, 2021

  • 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).

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

backbone87 commented May 8, 2021

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

@backbone87 backbone87 force-pushed the feature/share-reset-notifier branch 2 times, most recently from 105524f to ba0cffc Compare May 8, 2021 04:45
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

spec/operators/share-spec.ts Outdated Show resolved Hide resolved
@backbone87
Copy link
Contributor Author

backbone87 commented May 8, 2021

  • 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 backbone87 force-pushed the feature/share-reset-notifier branch from ba0cffc to 182ee11 Compare May 8, 2021 14:01
@backbone87
Copy link
Contributor Author

added some cleanups to the impl:

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

@cartant
Copy link
Collaborator

cartant commented May 8, 2021

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.

/**
* 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 backbone87 marked this pull request as ready for review May 9, 2021 03:40
@backbone87 backbone87 force-pushed the feature/share-reset-notifier branch from 778a9e7 to ba65f6a Compare May 9, 2021 03:43
@backbone87
Copy link
Contributor Author

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

@backbone87
Copy link
Contributor Author

backbone87 commented May 9, 2021

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
Copy link
Collaborator

cartant commented May 9, 2021

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
Copy link
Member

benlesh commented May 10, 2021

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
Copy link
Contributor Author

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 backbone87 force-pushed the feature/share-reset-notifier branch from ba65f6a to d1447f4 Compare May 10, 2021 19:13
@backbone87
Copy link
Contributor Author

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.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/internal/operators/share.ts Show resolved Hide resolved
@benlesh
Copy link
Member

benlesh commented May 20, 2021

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
Copy link
Member

benlesh commented May 20, 2021

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 benlesh merged commit 12c3716 into ReactiveX:master May 20, 2021
@benlesh
Copy link
Member

benlesh commented May 20, 2021

Thank you, @backbone87, for this great contribution!

@backbone87 backbone87 deleted the feature/share-reset-notifier branch May 29, 2021 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.x Issues and PRs for version 7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refCount with delay
3 participants