Skip to content
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 Make kernel message handling async and in order #4697

Merged
merged 79 commits into from Jun 27, 2018

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 8, 2018

Fixes #4188

  • Fix tests
  • Read through to audit one more time
@jasongrout jasongrout added this to the Beta 3 milestone Jun 8, 2018
@jasongrout jasongrout force-pushed the asyncmessages branch 3 times, most recently from 40e6302 to 13e9d51 Jun 12, 2018
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 12, 2018

Rebased on current master so the test output is cleaner, without spurious warnings.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

After an initial pass, this is looking to be in good shape. My main takeaway is how much easier some of this stuff is to read with async/await.

I'm glad to see tests on your TODOs, there are a lot of API changes here.

try {
continueHandling = hook(msg);
continueHandling = await hook(msg);
Copy link
Member

@ian-r-rose ian-r-rose Jun 13, 2018

My oh my, is chaining promises easier to read with async/await.

Copy link
Contributor Author

@jasongrout jasongrout Jun 13, 2018

:)

* provide a removeCommTarget function instead of returning a disposable.
* Presumably it's just as easy for someone to store the comm target name as
* it is to store the disposable. Since there is only one callback, you don't even
* need to store the callback.
Copy link
Member

@ian-r-rose ian-r-rose Jun 13, 2018

One very concrete difference between a remove method vs the disposable approach is that with the latter, it is impossible for anybody other than the initial registrar to remove the item. If you don't have access to the disposable, you will never be able to delete a target. With an ID, it is possible for anybody to delete it by ID.

It may not matter much here, since the IDs are opaque and non-readable. The disposable pattern has rather large consequences for the commands and keyboard shortcuts, however.

Copy link
Contributor Author

@jasongrout jasongrout Jun 13, 2018

Good point, thanks for the reminder. I think you're probably right that it should not have any effect here.

*/
connectToComm(targetName: string, commId?: string): Promise<Kernel.IComm>;
connectToComm(targetName: string, commId?: string): Kernel.IComm;
Copy link
Member

@ian-r-rose ian-r-rose Jun 13, 2018

Why did this function become synchronous?

Copy link
Contributor Author

@jasongrout jasongrout Jun 13, 2018

There's nothing asynchronous about it anymore. Since comms messages are now processed asynchronously, the actuals comms themselves are stored, rather than just promises to an eventually-created comm. So I can return the actual comm, not just a promise to a comm.

}

/**
* Connect to a running kernel.
*
* TODO: why is this function async?
Copy link
Member

@ian-r-rose ian-r-rose Jun 13, 2018

I'm not sure, but that fact has made some of the kernel help links in the menus a pain to add.

Copy link
Contributor Author

@jasongrout jasongrout Jun 13, 2018

I'm okay with making it sync, I think. I put the TODO there to come back to it after mulling over it/sleeping on it :).

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 13, 2018

Is the PromiseLike type signature used so that @jupyterlab/services users can use a polyfill if they so desire?

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 13, 2018

Is the PromiseLike type signature used so that @jupyterlab/services users can use a polyfill if they so desire?

It's just good practice. I don't care if it is actually a promise, just that it is resolvable. For example, all the TS typings for resolve, etc., use PromiseLike.

jasongrout added 11 commits Jun 22, 2018
This is so that code waiting on, for example, a status message will still resolve, especially in the case that the messages were buffered into another session.

Any message handling that may require the current session (such as creating comms) should check to see if the message being handled is current.
It’s still a bit puzzling what is happening - it appears that the notebook is dropping messages from the kernel. More details at jupyter/notebook#3705.
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 26, 2018

@ian-r-rose - I added a long test to test that all of the message handling is asynchronous and in order!

* See [Update Display data](https://jupyter-client.readthedocs.io/en/latest/messaging.html#update-display-data).
*/
export
interface IUpdateDisplayDataMsg extends IDisplayDataMsg {
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

Is this something that can be simplified with conditional types?

Copy link
Contributor Author

@jasongrout jasongrout Jun 26, 2018

I suppose we could use a MakeRequired typing thing, but I think it's simpler to be more explicit here.

@@ -1451,7 +1451,7 @@ namespace Private {
// We might want to move the handleRestart to after we get the response back

// Handle the restart on all of the kernels with the same id.
each(runningKernels, k => {
each(runningKernels.slice(), k => {
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

I don't see the disposal possibility here.

@@ -1463,7 +1463,7 @@ namespace Private {
let data = await response.json();
validate.validateModel(data);
// Reconnect the other kernels asynchronously, but don't wait for them.
each(runningKernels, k => {
each(runningKernels.slice(), k => {
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

Or here.

});

});

context('handles messages asynchronously', () => {
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

This is quite the test.



// TODO: Check tests for any kernels that are disposed before done
// let status = [];
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

Looks like leftover console logs.

export
async function testResolveOrder(promises: PromiseLike<any>[], resolutions: any[]): Promise<void> {
// We construct all of the races synchronously so that the winner can truly be determined
let subsequences = promises.map((value, index) => Promise.race(promises.slice(index)));
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

I just learned about Promise.race

Copy link
Contributor Author

@jasongrout jasongrout Jun 26, 2018

:)

* false if the promise is still pending.
*/
export
async function isFulfilled<T>(p: PromiseLike<T>): Promise<boolean> {
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

Very clever. I had thought there was no way to do this with JS. At least it's not synchronous, or else my whole world view would be challenged.

}
} catch (e) {
done.reject(e);
// throw e;
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

Dead code.

Copy link
Contributor Author

@jasongrout jasongrout Jun 26, 2018

deleted

done.resolve(options.value || undefined);
}
}, object);
return done.promise;
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

Why does this function need to be asychronous?

Copy link
Contributor Author

@jasongrout jasongrout Jun 26, 2018

I added a comment. Basically, the thing that effected the signal may be asynchronous, so this allows that thing to happen.

this._futures.forEach(future => { future.dispose(); });
this._comms.forEach(comm => { comm.dispose(); });
this._kernelSession = '';
this._msgChain = null;
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

How close are we to enabling strict null checks? Should we type msgChain as Promise<void> | null?

Copy link
Contributor Author

@jasongrout jasongrout Jun 26, 2018

done.

@@ -17,19 +17,30 @@ import {
KernelMessage
} from './messages';

// from Phosphor
export
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

Does this need to be exported?

Copy link
Contributor Author

@jasongrout jasongrout Jun 26, 2018

no. Fixed in the latest commit I just pushed.

*
* #### NOTES
* We can't just use requestAnimationFrame since it is not available in our
* testing setup. This implementation is from Phosphor:
Copy link
Member

@ian-r-rose ian-r-rose Jun 26, 2018

I would argue it's not just for testing: we want services to work in a node environment.

Copy link
Contributor Author

@jasongrout jasongrout Jun 26, 2018

Changing comment, then...you're right, it's really node that we're talking about here.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 27, 2018

Okay, let's merge and move on to the follow ups. Thanks for the herculean effort @jasongrout!

@ian-r-rose ian-r-rose merged commit d856a87 into jupyterlab:master Jun 27, 2018
2 checks passed
@afshin
Copy link
Member

@afshin afshin commented Jun 27, 2018

Fixes #4790

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants