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.

Sign up for GitHub

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

Jump to bottom

fix: NotificationManager should only invoke resource/document callbacks owned by the originating store #8649

Merged
merged 2 commits into from Jun 28, 2023
Merged

fix: NotificationManager should only invoke resource/document callbacks owned by the originating store #8649

merged 2 commits into from Jun 28, 2023

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Jun 28, 2023 •
edited

The underlying bug is that the NotificationManager was storing callbacks for resource and document in the global Cache where we only need to store the callbacks for actual identifiers.

While investigating I also noticed that RecordArrayManager.clear cleared a global cache that it did not own. So on top of fixing those things I also reviewed teardown for the NotificationManager global cache, and opted to make it more safely encapsulated.

@runspired runspired added 🎯 canary PR is targeting canary (default) 🏷️ chore This PR primarily refactors code or updates dependencies 🎯 beta PR should be backported to beta 🎯 release PR should be backported to release 🎯 lts The PR should be backported to the most recent LTS 🏷️ bug This PR primarily fixes a reported issue lts-4-12 Long Term LTS Maintenance and removed 🏷️ chore This PR primarily refactors code or updates dependencies labels Jun 28, 2023
@runspired runspired changed the title chore: attempt to isolate #8648 with a test fix: NotificationManager should only invoke resource/document callbacks owned by the originating store Jun 28, 2023
@runspired runspired linked an issue Jun 28, 2023 that may be closed by this pull request
Concurrent Fastboot requests throw Illegal set of identifier on cache peek #8648
Closed
@runspired runspired merged commit e6a0ee1 into main Jun 28, 2023
19 of 20 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fix-fastboot-concurrency branch June 28, 2023 07:36
runspired added a commit that referenced this pull request Jun 29, 2023
@runspired
fix: NotificationManager should only invoke resource/document callbac…
b77ebef 
…ks owned by the originating store (#8649)

* attempt to isolate #8648 with a test

* fixes and tests
@runspired runspired mentioned this pull request Jun 29, 2023
Merged
@runspired runspired removed the lts-4-12 Long Term LTS Maintenance label Jun 29, 2023
runspired added a commit that referenced this pull request Jun 29, 2023
@runspired
fix: NotificationManager should only invoke resource/document callbac…
da33538 
…ks owned by the originating store (#8649) (#8656)

* attempt to isolate #8648 with a test

* fixes and tests
runspired added a commit that referenced this pull request Jun 29, 2023
@runspired
fix: NotificationManager should only invoke resource/document callbac…
b2c2b72 
…ks owned by the originating store (#8649)

* attempt to isolate #8648 with a test

* fixes and tests
@gitKrystan gitKrystan mentioned this pull request Dec 6, 2023
Draft
3 tasks
@gitKrystan gitKrystan mentioned this pull request Jan 9, 2024
Merged
This was referenced Apr 29, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
🎯 beta PR should be backported to beta 🎯 canary PR is targeting canary (default) 🎯 lts The PR should be backported to the most recent LTS 🎯 release PR should be backported to release 🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Concurrent Fastboot requests throw Illegal set of identifier on cache peek
1 participant
@runspired