Merge pull request #3470 from maartenbreddels/fix_message_throttle_negative

fix: _pending_msgs was negative, iopub.status was called too often

Author: ibdafna

packages/base/src/widget.ts

@@ -334,6 +334,12 @@ export class WidgetModel extends Backbone.Model {
     if (this.comm !== void 0) {
       if (msg.content.execution_state === 'idle') {
         this._pending_msgs--;
+        if (this._pending_msgs < 0) {
+          console.error(
+            `Pending messages < 0 (=${this._pending_msgs}), which is unexpected`
+          );
+          this._pending_msgs = 0; // do not break message throttling in case of unexpected errors
+        }
         // Send buffer if one is waiting and we are below the throttle.
         if (this._msg_buffer !== null && this._pending_msgs < 1) {
           const msgId = this.send_sync_message(
@@ -548,11 +554,16 @@ export class WidgetModel extends Backbone.Model {
     }
     try {
       callbacks.iopub = callbacks.iopub || {};
-      const statuscb = callbacks.iopub.status;
+      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;
       callbacks.iopub.status = (msg: KernelMessage.IStatusMsg): void => {
         this._handle_status(msg);
-        if (statuscb) {
-          statuscb(msg);
+        if (callbacks.iopub.statusPrevious) {
+          callbacks.iopub.statusPrevious(msg);
         }
       };
 

packages/base/test/src/widget_test.ts

@@ -616,31 +616,99 @@ describe('WidgetModel', function () {
     });
 
     it('respects the message throttle', function () {
-      const send = sinon.spy(this.widget, 'send_sync_message');
+      // reuse the callback, similar to .touch in views, which will
+      // expose an bug of calling onstatus multiple times
+      const callbacks = this.widget.callbacks();
       this.widget.set('a', 'sync test');
-      this.widget.save_changes();
+      expect(this.widget._pending_msgs).to.equal(0);
+      this.widget.save_changes(callbacks);
+      expect(this.widget._pending_msgs).to.equal(1);
       this.widget.set('a', 'another sync test');
       this.widget.set('b', 'change b');
-      this.widget.save_changes();
+      this.widget.save_changes(callbacks);
       this.widget.set('b', 'change b again');
-      this.widget.save_changes();
+      this.widget.save_changes(callbacks);
+      expect(this.widget._pending_msgs).to.equal(1);
 
       // check that one sync message went through
-      expect(send).to.be.calledOnce;
-      expect(send).to.be.calledWith({
-        a: 'sync test',
+      expect(this.comm.send).to.be.calledOnce;
+      expect(this.comm.send).to.be.calledWith({
+        method: 'update',
+        state: { a: 'sync test' },
+        buffer_paths: [],
+      });
+      expect(this.widget._msg_buffer).to.deep.equal({
+        a: 'another sync test',
+        b: 'change b again',
       });
+      expect(this.widget._msg_buffer_callbacks).to.not.equals(null);
       // have the comm send a status idle message
-      this.widget._handle_status({
+      callbacks.iopub.status({
         content: {
           execution_state: 'idle',
         },
       });
+      // we directly get a new pending message
+      expect(this.widget._pending_msgs).to.equal(1);
+      expect(this.widget._msg_buffer).to.equal(null);
+      expect(this.widget._msg_buffer_callbacks).to.equals(null);
+
       // check that the other sync message went through with the updated values
-      expect(send.secondCall).to.be.calledWith({
-        a: 'another sync test',
-        b: 'change b again',
+      expect(this.comm.send.secondCall).to.be.calledWith({
+        method: 'update',
+        state: {
+          a: 'another sync test',
+          b: 'change b again',
+        },
+        buffer_paths: [],
+      });
+      callbacks.iopub.status({
+        content: {
+          execution_state: 'idle',
+        },
+      });
+      expect(this.widget._pending_msgs).to.equal(0);
+
+      // repeat again
+      this.comm.send.resetHistory();
+
+      this.widget.set('a', 'sync test - 2');
+      this.widget.save_changes(callbacks);
+      expect(this.widget._pending_msgs).to.equal(1);
+      this.widget.set('a', 'another sync test - 2');
+      this.widget.set('b', 'change b - 2');
+      this.widget.save_changes(callbacks);
+      this.widget.set('b', 'change b again - 2');
+      this.widget.save_changes(callbacks);
+      expect(this.widget._pending_msgs).to.equal(1);
+      expect(this.comm.send).to.be.calledOnce;
+      expect(this.comm.send).to.be.calledWith({
+        method: 'update',
+        state: { a: 'sync test - 2' },
+        buffer_paths: [],
+      });
+      callbacks.iopub.status({
+        content: {
+          execution_state: 'idle',
+        },
+      });
+      // again, directly a new message
+      expect(this.widget._pending_msgs).to.equal(1);
+
+      expect(this.comm.send.secondCall).to.be.calledWith({
+        method: 'update',
+        state: {
+          a: 'another sync test - 2',
+          b: 'change b again - 2',
+        },
+        buffer_paths: [],
+      });
+      callbacks.iopub.status({
+        content: {
+          execution_state: 'idle',
+        },
       });
+      expect(this.widget._pending_msgs).to.equal(0);
     });
 
     it('Initial values are *not* sent on creation', function () {