rwaldron / temporal

Non-blocking, temporal task sequencing. For use with robots built with Johnny-Five
http://johnny-five.io/
MIT License
84 stars 12 forks source link

Cannot rely on Object.keys order #19

Closed dtex closed 6 years ago

dtex commented 6 years ago

I wanted to submit this before submitting the HR pull request to avoid confusion.

When using Object.keys() to populate the scheduled array in processQueue() we can't trust that V8 will return an array that is sorted by key. Since temporal checks the last member of that array to see if there is anything left in the queue that needs to be run, temporal checks the last member added, not the last member by the scheduled time to execute.

In other words, Temporal assumes that tasks are added in the order that they are expected to execute.

For example, this works:

let temporal = require("temporal");

temporal.delay(5, () => { console.log(5);});
temporal.delay(10, () => { console.log(10);});
\\ 5
\\ 10

But this does not:

let temporal = require("temporal");

temporal.delay(10, () => { console.log(10);}); // Never fires
temporal.delay(5, () => { console.log(5);});
\\ 5

Here's the offending code:

var scheduled = Object.keys(queue);
var last = scheduled.length && +scheduled[scheduled.length - 1];
dtex commented 6 years ago

Storing last in a global and updating anytime task.later is greater seems to solve the problem. I'll do some performance testing on that option.

dtex commented 6 years ago

Here's all the dirt on this issue.

https://github.com/tc39/ecma262/pull/1242

In short, implementations don't match the spec.