Skip to content

Commit

Permalink
Fix #3469 by making a copy of the callbacks dict.
Browse files Browse the repository at this point in the history
This partially reverts changes from #3470.

I think the underlying assumptions around send_sync_message and callbacks are:

1. send_sync_message is only ever called once per message. When the original sync happens, either we put the message on the queue or we send it immediately, and the callback modifications happen only when we actually send the message.
2. There was an implicit assumption in the send_sync_message code is that the callbacks object passed in is a new object unique to this message, i.e., we aren't modifying a global object, that then we'll modify again later.

I think assumption 2 is sometimes not true, so this guards against it by making a (2-deep) copy of the callbacks object before modifying and using it.
  • Loading branch information
jasongrout committed Jun 11, 2022
1 parent d1491e9 commit d8e7fb6
Showing 1 changed file with 12 additions and 10 deletions.
22 changes: 12 additions & 10 deletions packages/base/src/widget.ts
Original file line number Diff line number Diff line change
@@ -334,9 +334,10 @@ export class WidgetModel extends Backbone.Model {
if (this.comm !== void 0) {
if (msg.content.execution_state === 'idle') {
this._pending_msgs--;
// Sanity check for logic errors that may push this below zero.
if (this._pending_msgs < 0) {
console.error(
`Pending messages < 0 (=${this._pending_msgs}), which is unexpected`
`Jupyter Widgets message throttle: Pending messages < 0 (=${this._pending_msgs}), which is unexpected. Resetting to 0 to continue.`
);
this._pending_msgs = 0; // do not break message throttling in case of unexpected errors
}
@@ -553,17 +554,18 @@ export class WidgetModel extends Backbone.Model {
return '';
}
try {
callbacks.iopub = callbacks.iopub || {};
if (callbacks.iopub.previouslyUsedByJupyterWidgets === undefined) {
// Do not break other code that also wants to listen to status updates
callbacks.iopub.statusPrevious = callbacks.iopub.status;
}
// else callbacks.iopub.status is the old handler, that we should not reuse
callbacks.iopub.previouslyUsedByJupyterWidgets = true;
// Make a 2-deep copy so we don't modify the caller's callbacks object.
callbacks = {
shell: { ...callbacks.shell },
iopub: { ...callbacks.iopub },
input: callbacks.input,
};
// Save the caller's status callback so we can call it after we handle the message.
const statuscb = callbacks.iopub.status;
callbacks.iopub.status = (msg: KernelMessage.IStatusMsg): void => {
this._handle_status(msg);
if (callbacks.iopub.statusPrevious) {
callbacks.iopub.statusPrevious(msg);
if (statuscb) {
statuscb(msg);
}
};

0 comments on commit d8e7fb6

Please sign in to comment.