-
Notifications
You must be signed in to change notification settings - Fork 234
Add ability to cancel tasks, and update to allow components to work in node #1230
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
Conversation
|
To test in the lab, be sure to run pnpm -s lab:srefirst, and add loader: {
paths: {
sre: './node_modules/mathjax-full/mjs/a11y/sre'
}
}to the configuration in |
…dd tests for that
…unneeded promises from startup.ts
Add core promise-based typeset and convert functions, and simplify those in Startup.
zorkow
left a comment
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 few minor things. And one point for discussion to improve the performance of cancel. But it is not a blocking concern.
| if (i > 0) { | ||
| this.tasks[i].reject(`Task ${this.tasks[i].cmd.cmd} cancelled`); | ||
| this.tasks.splice(i, 1); | ||
| } |
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.
This looks like it might be better suited to implement as a map to get push and shift operations. Since Maps preserve order of entry insertion, this should not be a problem using entries().next().
For the time being, I think it is probably good enough as I assume cancellation will be a rare event. But in case this should become a performance problem one day we could refactor this.
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.
I wasn't particularly happy about that, either, but didn't want to make bigger changes just for this. As you say, it is likely not going to come into play very often, and can be dealt with later.
Since
Maps preserve order of entry insertion
Is that guaranteed, or is it just the way things currently are? It sound like that could be a nice solution.
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.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map claims
The Map object holds key-value pairs and remembers the original insertion order of the keys.
According to the official spec insertion order is definitely required for foreach:
https://262.ecma-international.org/#sec-map.prototype.foreach
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.
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.
Thanks for the details. Looks like it should be a straight-forward change, then.
Overview
The main purposes of this PR are to:
It also changes how the worker handles promises. Rather than having the GeneratorPool create promises, the WorkerHandler's
Post()command now does that automatically, so every command send to the worker pool has a promise that resolves when that command is complete.This change also includes the ability to pass data back from the worker to the client via that promise. This allows the worker to send the speech structure data back via the promise rather than a client
Attach()command. The client now uses that data to do the attach action itself. This allows the worker to start processing other commands while the client is attaching the speech from the previous one, since the worker will have finished its command when it passes back the speech structure, rather than having to wait fo the client to attach and then finish.The Details
In the worker pool configuration file, we add the verisons check (so we don't get an error message about missing version information when a node application's
LiteIFrameloads the worker pool data viaasyncLoad()).We add the speech component to the
a11y/util.jsso that the combined components that include the assisitive tools will include the speech component.We extend the
asyncLoad()definition in the core component to potentially handle loading node packages (we do that in theLiteIFrameby loading thenode:modulenode package). In a node application that has set therequireconfiguration option properly to either the noderequireor to(file) => import(file), or an equivalent, this will allow loading of core node modules.The version of SRE in the
labdirectory now sets up the mathmaps path to work in the lab. (The typo that I had in theSREfeaturethat you corrected actually allowed it to work before, but now that that has been corrected, it no longer does, but this takes care of that.)The speech and Braille controls are removed from the semantic-enrich component, since they are not used there, and are already in the speech component, where they are used.
In
speech.ts, the GeneratorPool'sSpeech()function now returns a promise (see below), so we take advantage of that insteach of using a try/catch construction. We also push the promise into the document'srenderPromisesarray so thatMathJax.typesetPromise()will wait for it before resolving. That way, the speech will be on the DOM nodes when the typeset promise resolves (as it is in beta.7).The MathItem's
clear()method now cancels its speech task, if there is one.The MathDocument now gets a
done()method that terminates the worker pool.In
GeneratorPool.ts:Post()command of theWorkerHandlernow creates promises automatically (see below), we don't need to get our own promises.Promise<any>notPromise<void>.Speech()command now saves that promise and returns it (as donextRulesandnextStyle).cancel()method cancels any task saved for this MathItem.In
MessageTypes.ts, thePromiseFunctionsare no longer needed, and we add some types for the speech structure that will be passed back from the worker.In
WebWorker.ts:resolve()function can now pass an argument.Post()method now creates a promise for the task automatically.Speech()method is moved down to be next to thenextRulesandnextStylemethods.Cancel()method is added to allow a pending task to be canceled (removed from the task list) and its promise rejected. We might want to make a convention for this so that warnings aren't produced when tasks are canceled explicitly.Speech()command is now anasyncfunction that waits for the worker to return the speech structure and then callsAttach()on using that. (The worker no longer callsAttach()itself, but just returns the data, so it can go on to do other tasks while the speech is being attached.)nextRulesandnextStyle.Attach()function has been moved out the worker commands and is now just a regular method. It is also broken into three parts (rather than creating functions internally), andsetSpecialAttributes()is moved here along with the other separated outsetSpeechAttribute()andsetSpeechAttributes().Terminate()method now rejects all the tasks in the list, and before terminating the worker pool.Stop()method now waits for the termination to complete before removing the iframe.Finished()method now returns either the result from the worker, or an error message from it.In
speech-worker.ts:copyStructure()commands.then()andcatch()rather than a try/catch structure. An error state is indicated by anerrorproperty of themsgpassed toFinished()(see below).WorkerFunctionandWorkerResulttypes for more specificity.feature()command is removed, since SRE is already configured and loaded, so this would have no effect.setup()command now waits for the setup to take effect (though the command is not used currently).Finished()now takes amsgparameter that is sent back to the client. Thesuccessproperty is set according to whether there is an error message or not.Speech()action is simplified a bit, and now returns the speech structure data rather than asking the client to attach. The client gets the structure data from the promise it got when posting the worker command, and does the attach operation itself.The
LiteIFrameelement indicates thatasyncLoadneeds to load a node module so that theasyncLoadfrom thecorecomponent will not use the Package module's loader.The
loader.tsfile now allows theLoad()function to return the results of the load operations (i.e., the exported values of the packages that were loaded). This allows theLiteIFrameto get the exported values from thespeech-workerpoolwhen components are being used in node. The results are stored in newresultsproperties of thePackagedata (see below).In
package.ts, we now retain the result of loading a package, i.e., its exported values, so that the component loader can return those.In
startup.ts, we fix a typo with the document used for thetoMML()function (argh), and create a newMathJax.done()function that is used to terminate the worker pool, if it was started.In
MathDocument.ts:done()method that can be used to clean up anything when the document is no longer needed. (We use it to terminate the worker pool in the speech handler.)clearMathItemsWithin()now calls each MathItem'sclear()method (used to cancel its speech task, if any).In
MathItem.ts, we add the newclear()method, and call it from theremoveFromDocument()method, so that if a MathItem is removed, its tasks are canceled.Finally, in
HTMLMathItem.ts, we make sure the super-classremoveFromDocument()is called (so theclear()is performed).