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

Ensure adapters and serializers are destroyed upon store destruction. #7020

Merged

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jan 30, 2020

Prior to this change, adapters and serializers were never destroyed. Commonly (and anecdotally) this is "fine" (heck this is the first report of the issue in years!), but it is pretty common for adapters to do async of their own (e.g. tracking adapter responses, exponential backoffs, etc) and without .destroy() being called on them there is no way for those adapters/serializer to ensure that async is canceled appropriately.

Fixes #7018

@rwjblue rwjblue requested review from runspired and igorT January 30, 2020 23:27
@rwjblue rwjblue added lts-target 🎯 beta PR should be backported to beta 🎯 release PR should be backported to release labels Jan 30, 2020
@rwjblue rwjblue force-pushed the ensure-adapter-serializer-destruction branch from b02e70e to dab0d9c Compare January 30, 2020 23:29
* @public
* @optional
*/
destroy?(): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! <3

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
@rwjblue rwjblue force-pushed the ensure-adapter-serializer-destruction branch from dab0d9c to bf459de Compare January 30, 2020 23:37
@runspired runspired merged commit 13a67a2 into emberjs:master Jan 31, 2020
@rwjblue rwjblue deleted the ensure-adapter-serializer-destruction branch January 31, 2020 00:58
@HeroicEric HeroicEric added 🎯 lts The PR should be backported to the most recent LTS and removed lts-target labels Feb 20, 2020
HeroicEric pushed a commit that referenced this pull request Feb 27, 2020
…#7020)

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
@HeroicEric HeroicEric mentioned this pull request Feb 27, 2020
HeroicEric pushed a commit to HeroicEric/data that referenced this pull request Feb 27, 2020
…emberjs#7020)

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
@HeroicEric HeroicEric mentioned this pull request Feb 27, 2020
igorT pushed a commit that referenced this pull request Feb 28, 2020
…#7020)

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
igorT pushed a commit that referenced this pull request Feb 28, 2020
…#7020)

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
@HeroicEric HeroicEric removed 🎯 release PR should be backported to release 🎯 beta PR should be backported to beta labels Feb 28, 2020
HeroicEric pushed a commit to HeroicEric/data that referenced this pull request Mar 6, 2020
…emberjs#7020)

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
HeroicEric pushed a commit to HeroicEric/data that referenced this pull request Mar 6, 2020
…emberjs#7020)

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
igorT pushed a commit to HeroicEric/data that referenced this pull request Mar 9, 2020
…emberjs#7020)

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
igorT pushed a commit that referenced this pull request Mar 9, 2020
…#7020)

Prior to this change, adapters and serializers were never destroyed.
Commonly (and anecdotally) this is "fine" (heck this is the first report
of the issue in years!), but it is pretty common for adapters to do
async of their own (e.g. tracking adapter responses, exponential
backoffs, etc) and without `.destroy()` being called on them there is no
way for those adapters/serializer to ensure that async is canceled
appropriately.
@kiwiupover
Copy link

@rwjblue can we back port this fix to at least 3.11?

@igorT
Copy link
Member

igorT commented Mar 17, 2020

@kiwiupover 3.11 isn't an lts, would 3.12 work for you?

@kiwiupover
Copy link

@igorT because of this bug #6867

@runspired runspired removed the 🎯 lts The PR should be backported to the most recent LTS label Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adapter instances should be destroyed
6 participants