-
Notifications
You must be signed in to change notification settings - Fork 148
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
WIP async #70
WIP async #70
Conversation
…he minimized version in the submodule)
Any evaluation can now potentially return a promise, and higher-level routines now accomodate promises Change the tests to also check for promises In tests avoid EPSILON numeric tests when both numbers are integers
Each test case uses a separate interpreter instance so they should be isolated. The tests assume synchronous behavior though, which may be why they're flaky for you. That'll have to change to being promise-based, which will require reworking the use of QUnit (https://api.qunitjs.com/async/) and the helpers. |
I setup the tests so they are async, though I believe the tests become somewhat interleaved in the process. For instance, there's a test for The problem I am having with the tests is that I get 41 tests run in the Parser section, and no other tests. Here's what the tests look like to me: http://pageshot.dev.mozaws.net/mTG3Lb0YRafaKgBD/file – it could be some test setup problem I created, but there's nothing in the console. |
} | ||
|
||
function makePromise(value) { | ||
return isPromise(value) ? value : Promise.resolve(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can replace makePromise with just Promise.resolve(value)
var p = new Promise(function(){});
Promise.resolve(p) === p; // true
Serialize multiple calls to .run(), so they always run in order (not in parallel)
…he non-promise code paths
With c5918e0 the tests all seem to pass. It does it mostly be removing all "real" concurrency, so everything is serialized, including the tests. This is appropriate because we don't have any primitives for doing concurrent work (such as an event handler or something like setTimeout or setInterval). So while the scope and stream bugs are not displaying themselves, they still exist. (Well, maybe streams should be a jumble of all the concurrent actors.) |
Thanks, I'll try and take a look, time permitting. I've left a few inline comments on style where the Promise using code can be simplified. |
@@ -2443,11 +2622,11 @@ function LogoInterpreter(turtle, stream, savehook) | |||
}); | |||
|
|||
def("stop", function() { | |||
throw new Output(); | |||
return Promise.reject({special: "stop"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this simply be return Promise.reject(new Output());
?
But more importantly, if the procedure invoker does the right thing, this could still be throw new Output();
// assume proc and inputs
var p = new Promise(function(resolve) {
resolve(proc.apply(self, inputs));
});
- If proc returns a non-Promise, p will be a Promise resolved to that value
- If proc returns a Promise, p will be a Promise that resolves to the same value
- If proc throws, p will be a rejected Promise
Make movement take time and work with promises Return values through all the drawing functions Fix turtle.end so it happens after the promise
…ix a bug somewhere in the process. Add promiseFinally to give use an equivalent to try/finally
Track when the interepreter is running in the UI, and allow the user to stop execution
$('#clear').addEventListener('click', clear); | ||
|
||
function setRun(canRun) { | ||
if (canRun) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better approach is to set/clear a style on an ancestor (e.g. document.body.classList.add('running')
/ document.body.classList.remove('running')
) and then use CSS to control the visibility:
#run, .running #stop { display: block; }
#stop, .running #run { display: none; }
Here's a rewritten FOR that passes the Control Structure tests. UNTIL and DO.UNTIL need fixing still. def("for", function(control, statements) {
control = reparse(lexpr(control));
statements = reparse(lexpr(statements));
function sign(x) { return x < 0 ? -1 : x > 0 ? 1 : 0; }
var varname = sexpr(control.shift());
var start, limit, step, current;
return Promise.resolve(evaluateExpression(control))
.then(function(r) {
current = start = aexpr(r);
return evaluateExpression(control);
})
.then(function(r) {
limit = aexpr(r);
})
.then(function() {
return new Promise(function(resolve, reject) {
function runLoop() {
if (sign(current - limit) === sign(step)) {
resolve();
return;
}
setvar(varname, current);
self.execute(statements)
.then(function() {
return (control.length) ?
evaluateExpression(control.slice()) : sign(limit - start);
})
.then(function(result) {
step = aexpr(result);
current += step;
runLoop();
})
.catch(reject);
}
runLoop();
});
});
}); |
Simplified DO.WHILE and WHILE: def("do.while", function(block, tf) {
block = checkevalblock(block);
return new Promise(function(resolve, reject) {
(function loop() {
self.execute(block)
.then(tf)
.then(function(cond) {
if (!cond) {
resolve();
return;
}
loop();
})
.catch(reject);
}());
});
}, {noeval: true});
def("while", function(tf, block) {
block = checkevalblock(block);
return new Promise(function(resolve, reject) {
(function loop() {
Promise.resolve(tf())
.then(function(cond) {
if (!cond) {
resolve();
return;
}
self.execute(block)
.then(loop);
})
.catch(reject);
}());
});
}, {noeval: true}); And working DO.UNTIL and UNTIL: function notpromise(tf) {
return Promise.resolve(tf()).then(function(r) { return !r; });
}
def("do.until", function(block, tf) {
var nottf = function () { return notpromise(tf); };
return self.routines.get("do.while")(block, nottf);
}, {noeval: true});
def("until", function(tf, block) {
var nottf = function () { return notpromise(tf); };
return self.routines.get("while")(nottf, block);
}, {noeval: true}); |
CASE is fine already, no work needed. Here's COND def("cond", function(clauses) {
clauses = lexpr(clauses);
return new Promise(function(resolve, reject) {
(function loop() {
if (!clauses.length) {
resolve();
return;
}
var clause = lexpr(clauses.shift());
var first = clause.shift();
if (isKeyword(first, 'ELSE')) {
resolve(evaluateExpression(clause));
return;
}
evaluateExpression(reparse(lexpr(first)))
.then(function(result) {
if (result) {
resolve(evaluateExpression(clause));
return;
}
loop();
})
.catch(reject);
}());
});
}); |
... and that gets all of the Control Structure tests passing. The Regression Tests also all pass except for
... but I'm done for the night. |
WIP - enable most tests, fix most procs and run loop
Add task queue API, revert slow turtle; all tests pass
Async execution - procedures may return Promises
This is a start at addressing #21
For some reason only some of the tests appear to me after I reload tests.html a few times, I don't understand quite what is happening.
This doesn't attempt to change index.js and the UI, it only attempts to address the tests currently. There are failing tests, I think mostly around serializing access to streams between multiple tests, and in scope. Scope I suspect is quite broken; as it should be, I didn't touch it. I really don't know how to handle scope. Maybe trying to send it through to routines explicitly as a
this
argument? The way it works currently I think just can't work in an async/concurrent environment.Also BYE and STOP are totally unaddressed, and OUTPUT is kind of fuzzy. I'm doing them as special ways of rejecting a promise now, which maybe just means at the top level there needs to be better catching of those rejections – but I don't quite understand what the "top" level is. I think in the case of OUTPUT I actually found the appropriate place. Is there nothing like
FOREVER [IF cond [BREAK]]
?There's a couple other FIXMEs of stuff I knowingly didn't do.