currentQueue is null in drainQueue

[DanFu09]

I'm getting an error in Firefox where currentQueue is null inside of drainQueue in babel-core/browser.js. From stepping through the code, there appears to be a race condition between two instances of drainQueue running at once. This is what is happening:

1. drainQueue starts the first time: queueIndex is -1, queue has one item in it
2. draining is set to true
3. setTimeout(cleanUpNextTick) called
4. currentQueue is set to queue, queue is set to an empty array
5. queueIndex compared to length of queue and then incremented
6. currentQueue[queueIndex].run() called
7. At this point, control goes to the cleanUpNextTick scheduled by drainQueue
8. draining set to false
9. cleanUpNextTick sets queue to currentQueue and calls drainQueue, starting a second instance of drainQueue
10. currentQueue set to a queue (which is just what it was before
11. Because queueIndex is already 1, no new functions are run
12. queueIndex set to -1
13. currentQueue is set to null
14. Second drainQueue completes
15. First drainQueue regains control
16. queueIndex is -1, because of second drainQueue
17. queueIndex is less than len, so currentQueue[queueIndex].run() is called
18. Exception because currentQueue is null

I've only observed this problem in Firefox, and usually only under very specific conditions (usually only the first time the page is loaded). We think this is causing problems for us downstream with Babel, Reflux, and React, so a fix would be greatly appreciated!

[calvinmetcalf]

the only way this could come up, would be if we were in an environment which redefined setTimeout in terms of process.nextTick which meant that the clean up function we thought would only run if the loop exited prematurely actually ran during the look because setTimeout was actually an alias for process.nextTick.

do Babel, Reflux, or React happen to set their own versions of setTimeout? if So why doesn't this usage cause a problem, the only difference is that it has the 0 explicitly stated, I wonder if changing this line to be

var timeout = setTimeout(cleanUpNextTick, 0);

or even

var timeout = setTimeout(cleanUpNextTick, 4);

(which should be equivalent in a browser) would fix it.

[deoxxa]

I'm seeing a similar problem in an environment that I'm putting together (ottoext) on top of the otto JavaScript interpreter. That interpreter, as with many others, doesn't contain an event loop. The way that I've implemented setTimeout and friends means that setTimeout(fn) is exactly the same as process.nextTick(fn).

There's a crash in cleanUpNextTick that has its roots in the same place as this one, which can also be papered over by changing setTimeout(cleanUpNextTick) to setTimeout(cleanUpNextTick, 5). Inside cleanUpNextTick, currentQueue can be null sometimes. I presume it's by the same mechanism as described above.

Perhaps a null check would be good in cleanUpNextTick as well? Either way, I'm going to find out what the correct behaviour is for setTimeout and make sure I implement that, but it could save future JavaScript environment tinkerers some time tracking down mysterious crashes.

[calvinmetcalf]

Omitting the number from setTimeout should be equivalent to setTimeout(fun,
4)

On Sat, Oct 3, 2015, 11:41 PM Conrad Pankoff notifications@github.com
wrote:

I'm seeing a similar problem in an environment that I'm putting together (
ottoext http://fknsrs.biz/p/ottoext) on top of the otto
https://github.com/robertkrimen/otto JavaScript interpreter. That
interpreter, as with many others, doesn't contain an event loop. The way
that I've implemented setTimeout and friends means that setTimeout(fn) is
exactly the same as process.nextTick(fn).

There's a crash in cleanUpNextTick that has its roots in the same place
as this one, which can also be papered over by changing
setTimeout(cleanUpNextTick) to setTimeout(cleanUpNextTick, 5). Inside
cleanUpNextTick, currentQueue can be null sometimes. I presume it's by
the same mechanism as described above.

Perhaps a null check would be good in cleanUpNextTick as well? Either
way, I'm going to find out what the correct behaviour is for setTimeout
and make sure I implement that, but it could save future JavaScript
environment tinkerers some time tracking down mysterious crashes.

—
Reply to this email directly or view it on GitHub
#49 (comment)
.

[deoxxa]

Yep, that's the behaviour I ended up implementing. I still had very, very occasional crashes at the same point, but they were certainly less frequent. I ended up managing to avoid the problem entirely by implementing setImmediate on my own and telling webpack (which was pulling this in, indirectly) not to shim the process module.

[calvinmetcalf]

If currentQueue is null in cleanUpNextTick that would likely be a seperate thing that would be worth opening it's own issue for. Off the top of my head it could be caused by clearTimeout not effectively removing the timeouts?