fix(router): complete router `events` on dispose by arturovt · Pull Request #59327 · angular/angular · GitHub
Skip to content

fix(router): complete router events on dispose - #59327

Closed
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/router-events
Closed

arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/router-events

Conversation

@arturovt

Copy link
Copy Markdown
Contributor

In this commit, we manually complete the events subject to remove all active observers and enable granular garbage collection, as users may forget to unsubscribe manually when subscribing to router.events.

@ngbot ngbot Bot added this to the Backlog milestone Dec 28, 2024
@arturovt
arturovt marked this pull request as ready for review December 28, 2024 18:34
@pullapprove
pullapprove Bot requested a review from crisbeto December 28, 2024 18:34
@crisbeto crisbeto added target: patch This PR is targeted for the next patch release action: global presubmit The PR is in need of a google3 global presubmit and removed action: global presubmit The PR is in need of a google3 global presubmit labels Dec 29, 2024
@crisbeto

Copy link
Copy Markdown
Member

@arturovt

Copy link
Copy Markdown
Contributor Author

@crisbeto what do you think if we call unsubscribe() on the subject in order to clean up the list of observers (w/o calling complete() on observers)? This is only to prevent any breaking changes. I could leave a comment that calling complete() is unsafe.

@crisbeto

Copy link
Copy Markdown
Member

We can try it out to see if it breaks anything internally.

In this commit, we manually complete the `events` subject to remove all active observers and enable
granular garbage collection, as users may forget to unsubscribe manually when subscribing to `router.events`.
@arturovt

Copy link
Copy Markdown
Contributor Author

Changed to unsubscribe().

@angular-robot
angular-robot Bot requested a review from crisbeto December 29, 2024 22:15
@crisbeto crisbeto added the action: global presubmit The PR is in need of a google3 global presubmit label Dec 29, 2024
@crisbeto

Copy link
Copy Markdown
Member

This time there were only two test failures on a global run, although I'm not quite sure why it's failing. Here's the stack trace:

ObjectUnsubscribedError: object unsubscribed

error properties: null({ constructor: Function })
Error
at _super (rxjs/src/internal/util/createErrorClass.ts:33:22)
      at new ObjectUnsubscribedErrorImpl (rxjs/src/internal/util/ObjectUnsubscribedError.ts:40:7)
      at Subject._throwIfClosed (rxjs/src/internal/Subject.ts:69:13)
      at Subject._trySubscribe (rxjs/src/internal/Subject.ts:134:10)
      at Subject.subscribe (rxjs/src/internal/Observable.ts:242:16)

Interestingly, these tests pass if I switch the unsubscribe call back to complete which had much more test failures. For reference, the code it's failing for looks something like this:

const currentPath = router.events.pipe(
  filter(event => event instanceof NavigationEnd),
  map(() => location.path()),
  startWith(location.path()),
);

@arturovt

Copy link
Copy Markdown
Contributor Author

Seems like the subscription happens after the router is disposed (the root injector is destroyed)...

@crisbeto

crisbeto commented Jan 3, 2025

Copy link
Copy Markdown
Member

I've sent out a fix for the problematic test. Once it lands, I'll re-run all the tests to make sure that nothing else regressed.

@crisbeto

crisbeto commented Jan 7, 2025

Copy link
Copy Markdown
Member

Passing TGP

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed action: global presubmit The PR is in need of a google3 global presubmit target: patch This PR is targeted for the next patch release labels Jan 7, 2025
@crisbeto

crisbeto commented Jan 7, 2025

Copy link
Copy Markdown
Member

I'm bumping it to the minor branch just in case some external apps depend on subscribing after destroy.

@thePunderWoman

Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit 52a6710.

The changes were merged into the following branches: main

@arturovt
arturovt deleted the fix/router-events branch January 7, 2025 17:09
@angular-automatic-lock-bot

Copy link
Copy Markdown

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Feb 7, 2025
PrajaktaB27 pushed a commit to PrajaktaB27/angular that referenced this pull request Feb 7, 2025
In this commit, we manually complete the `events` subject to remove all active observers and enable
granular garbage collection, as users may forget to unsubscribe manually when subscribing to `router.events`.

PR Close angular#59327
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: router target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants