WIP Make kernel message handling async and in order #4697

jasongrout · 2018-06-08T08:10:54Z

Fixes #4188

Fix tests
Read through to audit one more time

jasongrout · 2018-06-12T08:06:44Z

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

…serving message order. Fixes jupyterlab#4188

… processing of old kernel messages in the async queue.

…ore like an assert.

This also fixes at least one bug, where the shutdownAll was resolving before each shutdown actually resolved.

ian-r-rose

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.

ian-r-rose · 2018-06-13T21:58:02Z

packages/services/src/kernel/future.ts

        try {
-          continueHandling = hook(msg);
+          continueHandling = await hook(msg);


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

ian-r-rose · 2018-06-13T22:10:53Z

packages/services/src/kernel/default.ts

+   * 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.


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.

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

ian-r-rose · 2018-06-13T22:20:01Z

packages/services/src/kernel/kernel.ts

     */
-    connectToComm(targetName: string, commId?: string): Promise<Kernel.IComm>;
+    connectToComm(targetName: string, commId?: string): Kernel.IComm;


Why did this function become synchronous?

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.

ian-r-rose · 2018-06-13T22:23:12Z

packages/services/src/kernel/default.ts

  }

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


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

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 · 2018-06-13T22:25:51Z

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

jasongrout · 2018-06-13T22:28:40Z

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.

…eleting one from the list.

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 · 2018-06-26T16:22:20Z

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

ian-r-rose · 2018-06-26T19:15:39Z

packages/services/src/kernel/messages.ts

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


Is this something that can be simplified with conditional types?

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

ian-r-rose · 2018-06-26T19:17:29Z

packages/services/src/kernel/default.ts

@@ -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 => {


I don't see the disposal possibility here.

ian-r-rose · 2018-06-26T19:17:37Z

packages/services/src/kernel/default.ts

@@ -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 => {


ian-r-rose · 2018-06-26T19:20:26Z

packages/services/test/src/kernel/ikernel.spec.ts

    });

  });

+  context('handles messages asynchronously', () => {


This is quite the test.

ian-r-rose · 2018-06-26T19:26:25Z

packages/services/src/kernel/future.ts

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


Looks like leftover console logs.

ian-r-rose · 2018-06-26T19:39:55Z

packages/services/test/src/utils.ts

+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)));


I just learned about Promise.race

ian-r-rose · 2018-06-26T19:42:44Z

packages/services/test/src/utils.ts

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


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.

ian-r-rose · 2018-06-26T19:45:44Z

packages/services/test/src/utils.ts

+        }
+      } catch (e) {
+        done.reject(e);
+        // throw e;


ian-r-rose · 2018-06-26T19:46:12Z

packages/services/test/src/utils.ts

+      done.resolve(options.value || undefined);
+    }
+  }, object);
+  return done.promise;


Why does this function need to be asychronous?

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

ian-r-rose · 2018-06-26T19:52:15Z

packages/services/src/kernel/default.ts

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


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

ian-r-rose · 2018-06-26T21:55:47Z

packages/services/src/kernel/future.ts

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

+// from Phosphor
+export


Does this need to be exported?

no. Fixed in the latest commit I just pushed.

ian-r-rose · 2018-06-26T22:25:31Z

packages/services/src/kernel/future.ts

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


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

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

ian-r-rose · 2018-06-27T02:38:04Z

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

afshin · 2018-06-27T14:47:01Z

Fixes #4790

jasongrout added this to the Beta 3 milestone Jun 8, 2018

jasongrout force-pushed the asyncmessages branch 3 times, most recently from 40e6302 to 13e9d51 Compare June 12, 2018 08:06

jasongrout added 4 commits June 12, 2018 13:31

First pass at making all kernel message handling asynchronous and pre…

6faa2db

…serving message order. Fixes jupyterlab#4188

First pass at handling the case for restarting a kernel and canceling…

6d6425b

… processing of old kernel messages in the async queue.

Fix comm target callback signature.

6b88d89

WIP update docs

e905dd8

jasongrout force-pushed the asyncmessages branch from 13e9d51 to e905dd8 Compare June 12, 2018 20:31

jasongrout added 15 commits June 12, 2018 13:41

Make the IAnyMessage message readonly.

958f91f

Make connectToComm synchronous.

d7ea1bb

WIP more async kernel message processing work.

4b545d9

Deal with more async message hook processing.

c43f2e3

Rename checkCurrentMessage to assertCurrentMessage since it behaves m…

0112a70

…ore like an assert.

Convert kernel utility functions to use async/await syntax.

a710f4b

This also fixes at least one bug, where the shutdownAll was resolving before each shutdown actually resolved.

Standardize on async return types being: value | Promise<value>

17b7aa7

Better typing: Promise<void> -> PromiseLike<void>

ee17a1f

The future done promise returns the reply message, so use it.

086c935

More value | Promise<value> -> value | PromiseLike<value> changes.

8a5c58a

Rate-limit hook list compactions with requestAnimationFrame.

987bff9

Add/enhance documentation and comments.

6ae2a93

Documentation updates and code reorganization.

ba3e81e

Fix an anachronism in the code.

b206730

Message hooks can return promises, so update the type signature.

cb2b5f8

ian-r-rose reviewed Jun 13, 2018

View reviewed changes

jasongrout added 2 commits June 13, 2018 18:08

Fix some easier TODO items.

0c2baf2

Add removeCommTarget and removeMessageHook functions to kernels.

dd50e93

jasongrout added 11 commits June 21, 2018 17:08

Make a copy of the runningKernels list if there is a possibility of d…

9fdeae5

…eleting one from the list.

Work around issues where tests were sporadically hanging.

fc6b562

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.

Put back in debug logging.

9b4c9ab

Add explanatory notes.

f44dde6

Add testResolveOrder and isFulfilled utility functions, and tests.

989d66d

Kernel message hooks should also accept promises

b40abcd

Add some async message hooks in the standard tests.

bdcf02f

Add some tests for patterns, such as testing order of signal emissions.

652a26a

Add asynchronous message handling test, with associated infrastructure.

d96044b

Add transient message data for display updates to the message spec.

67f271e

jasongrout added 4 commits June 26, 2018 10:37

Remove kernel message logging

be6a65a

Definitely start a new kernel before each ikernel test.

0f3aae2

Fix failing kernel test.

353779f

Some minor cleanup.

bef6212

ian-r-rose reviewed Jun 26, 2018

View reviewed changes

jasongrout added 2 commits June 26, 2018 13:51

Cleanup based on review comments by @ian-r-rose

4a30e0f

Remove testResolveOrder since we did not end up using it.

8b7b059

ian-r-rose reviewed Jun 26, 2018

View reviewed changes

jasongrout added 2 commits June 26, 2018 15:16

Move the defer function to where it is being used.

0c45d74

Fix unhandled message test.

38e96a1

ian-r-rose reviewed Jun 26, 2018

View reviewed changes

Fix comment to refer to node.

58ecc9c

ian-r-rose merged commit d856a87 into jupyterlab:master Jun 27, 2018

afshin mentioned this pull request Jun 27, 2018

Unhandled promise rejection in future.js of @jupyterlab/services #4790

Closed

lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 8, 2019

lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Make kernel message handling async and in order #4697

WIP Make kernel message handling async and in order #4697

jasongrout commented Jun 8, 2018 •

edited

jasongrout commented Jun 12, 2018

ian-r-rose left a comment

ian-r-rose Jun 13, 2018

jasongrout Jun 13, 2018

ian-r-rose Jun 13, 2018

jasongrout Jun 13, 2018

ian-r-rose Jun 13, 2018

jasongrout Jun 13, 2018

ian-r-rose Jun 13, 2018

jasongrout Jun 13, 2018

ian-r-rose commented Jun 13, 2018

jasongrout commented Jun 13, 2018

jasongrout commented Jun 26, 2018 •

edited

ian-r-rose Jun 26, 2018

jasongrout Jun 26, 2018

ian-r-rose Jun 26, 2018

ian-r-rose Jun 26, 2018

ian-r-rose Jun 26, 2018

ian-r-rose Jun 26, 2018

ian-r-rose Jun 26, 2018

jasongrout Jun 26, 2018

ian-r-rose Jun 26, 2018

ian-r-rose Jun 26, 2018

jasongrout Jun 26, 2018

ian-r-rose Jun 26, 2018

jasongrout Jun 26, 2018

ian-r-rose Jun 26, 2018

jasongrout Jun 26, 2018

ian-r-rose Jun 26, 2018

jasongrout Jun 26, 2018

ian-r-rose Jun 26, 2018

jasongrout Jun 26, 2018

ian-r-rose commented Jun 27, 2018

afshin commented Jun 27, 2018



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

WIP Make kernel message handling async and in order #4697

WIP Make kernel message handling async and in order #4697

Conversation

jasongrout commented Jun 8, 2018 • edited

jasongrout commented Jun 12, 2018

ian-r-rose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-r-rose commented Jun 13, 2018

jasongrout commented Jun 13, 2018

jasongrout commented Jun 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-r-rose commented Jun 27, 2018

afshin commented Jun 27, 2018

jasongrout commented Jun 8, 2018 •

edited

jasongrout commented Jun 26, 2018 •

edited