Skip to content

Conversation

@jasongrout
Copy link
Member

A problem can happen when two messages come in for different
comms, where the second depends on the first (for example, the
first might be a message setting the state of a widget, and the
second triggering a view creation for the widget). Since comm
message queues are independent of each other, the second message
could be executed before the first message.

This exposes a more fundamental assumption users are likely to
have that messages from python are processed synchronously.

This includes #7780.

@jasongrout
Copy link
Member Author

We probably only need to merge this or #7839, but not both. This approach makes stronger guarantees about synchronicity of message processing.

@jasongrout
Copy link
Member Author

CCing @jdfreder, @minrk for comments

@jdfreder jdfreder added this to the 3.1 milestone Feb 24, 2015
@jdfreder
Copy link
Contributor

This may be a better approach for 4.0 (or later), but I think for 3.x the simplicity and minimal impact of #7839 is preferred. @minrk opinion?

@jasongrout
Copy link
Member Author

I think this is better than #7839 for the following reasons:

  1. in python, there really is an assumption of synchronous execution. This is conceptually simpler than the Handle CommManager messages in order #7839, as it just insists that all message handling be synchronous. We already have messages deserialized synchronously; this makes the entire execution synchronous.
  2. It's simpler to reason about the payload handlers if they are also executing synchronously, and this also guarantees that if they return a promise, they are executed before the next messages are processed. That's a nice thing to have.

A message handler doesn't have to be synchronous, of course. If it doesn't return a promise, we won't wait for it. This just makes the default to wait for handling before trying to handle more messages.

@minrk
Copy link
Member

minrk commented Feb 25, 2015

After a day of thinking about it, I'm inclined to agree. @jdfreder this PR is smaller than it looks, since ~half of it is actually #7780. The actual changes are here. It's mostly adding returns statements.

@jdfreder
Copy link
Contributor

jdfreder commented Mar 2, 2015

Since 7780 was merged, this is probably the best solution.

@minrk
Copy link
Member

minrk commented Mar 2, 2015

@jasongrout Can you rebase this one now that #7780 is merged so this accurately represents the diff?

A problem can happen when two messages come in for different
comms, where the second depends on the first (for example, the
first might be a message setting the state of a widget, and the
second triggering a view creation for the widget).  Since comm
message queues are independent of each other, the second message
could be executed before the first message.

This exposes a more fundamental assumption users are likely to
have that messages from python are processed synchronously.

Thanks to @dmadeka for reporting an error that led to discovering this issue.
@jasongrout jasongrout force-pushed the kernel-messages-sync branch from dcd10fa to f936f88 Compare March 3, 2015 16:01
@jasongrout
Copy link
Member Author

@minrk: rebased.

@minrk
Copy link
Member

minrk commented Mar 3, 2015

@jasongrout thanks!

minrk added a commit that referenced this pull request Mar 3, 2015
Keep kernel messages synchronous

promises all the way down
@minrk minrk merged commit ce1c3cf into ipython:master Mar 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants