trashable icon indicating copy to clipboard operation
trashable copied to clipboard

Potential race condition?

Open glasser opened this issue 6 years ago • 3 comments

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 avatar Nov 30 '17 02:11 glasser

See https://github.com/hjylewis/trashable/pull/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.

glasser avatar Nov 30 '17 02:11 glasser

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!

hjylewis avatar Nov 30 '17 21:11 hjylewis

That sounds scary but maybe it would work.

glasser avatar Dec 01 '17 18:12 glasser