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

Potential race condition? #6

Open
glasser opened this issue Nov 30, 2017 · 3 comments
Open

Potential race condition? #6

glasser opened this issue Nov 30, 2017 · 3 comments

Comments

@glasser
Copy link

glasser commented Nov 30, 2017

I like the idea behind this package.

However, I'm concerned that there's a potential race condition.

Let's consider three Promises. The "original" promise is the one that's running an async operation. The "trashable" promise is the one returned by makeTrashable(original). The "dangerous" promise is the one that runs some sort of code we want to avoid running if the trashable Promise was trashed, eg setState. ie:

const original = getSomethingAsync();
const trashable = makeTrashable(original);
const dangerous = trashable.then(x => c.setState({x});

So what if this happens:

  • original fulfills.
  • The then which trashable defines on original is called, and it calls resolve(val).
  • If I understand correctly, this does NOT immediately and synchronously call the x => c.setState({x}) function above. Instead, it enqueues a job onto the Promise Job Queue which says that this should be run later.
  • Potentially lots of other stuff happens here! I'm not sure if it's legal for JS to do stuff that isn't on the Promise Job Queue here, but for all we know React uses Promises itself and lots of React stuff could happen now.
  • Specifically, the React component is unmounted and we trash trashable.
  • Now that job above gets called, and setState gets run even though we already trashed the component.

Is this race condition impossible for some reason that I'm missing?

It seems like trashable/trashable-react will work well for nearly all of the times preventing us from doing the actual extra work that we're trying to avoid doing after unmount, but I'm not sure it will 100% of the time prevent us from getting that nasty setState warning.

@glasser
Copy link
Author

glasser commented Nov 30, 2017

See #7 for a demonstration. I suppose you could say "well, don't trash from a then in that particular place" but I'm not sure you can prove that it's impossible for random other code to run there.

@hjylewis
Copy link
Owner

Very interesting! Seems like what you are saying is possible. Thanks for explaining it so well.

I'll have to think about this some more and if there's a way to avoid the race (i.e. synchronously call the handler on the trashable promise) without losing garbage collectability.

I think there might be a way of making it so makeTrashable returns an object with the same interface of a promise (then and catch functions) but calls the handlers synchronously on resolve/reject instead of putting it on the Promise Job Queue... In other words, makeTrashable wraps the promise in a "pseudo-promise" instead of another promise.

Let me know if you have any thoughts!

@glasser
Copy link
Author

glasser commented Dec 1, 2017

That sounds scary but maybe it would work.

filipemir added a commit to filipemir/reactjs.org that referenced this issue Nov 12, 2019
As discussed in [this thread](facebook/react#5465 (comment)) and expanded [here](https://github.com/hjylewis/trashable/blob/master/PROOF.md), the solution proposed for cancelling promises when a component unmounts doesn't actually address the possible memory leaks. Interestingly, I don't believe @hjylewis's approach does so fully either (see [this issue](hjylewis/trashable#6)) but it does so in the vast majority of cases. If I'm following the conversation along correctly, I think the docs should be updated to reflect the better answer. And if it's not the better answer, I would love to hear the arguments against it.
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

No branches or pull requests

2 participants