Skip to content

Commit

Permalink
Merge pull request #3407 from jasongrout/testnoecho
Browse files Browse the repository at this point in the history 
Follow up on echo updates
  • Loading branch information
@vidartf
vidartf authored Mar 8, 2022
2 parents 3b2e20d + 94ffe26 commit ac7c4b9
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 116 deletions.
25 changes: 14 additions & 11 deletions packages/base/src/widget.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ export class WidgetModel extends Backbone.Model {
attributes: Backbone.ObjectHash,
options: IBackboneModelOptions
): void {
this.expectedEchoMsgIds = new Map<string, string>();
this.attrsToUpdate = new Set<string>();
this._expectedEchoMsgIds = new Map<string, string>();
this._attrsToUpdate = new Set<string>();

super.initialize(attributes, options);

Expand Down Expand Up @@ -237,18 +237,18 @@ export class WidgetModel extends Backbone.Model {
// we may have echos coming from other clients, we only care about
// dropping echos for which we expected a reply
const expectedEcho = Object.keys(state).filter((attrName) =>
this.expectedEchoMsgIds.has(attrName)
this._expectedEchoMsgIds.has(attrName)
);
expectedEcho.forEach((attrName: string) => {
// Skip echo messages until we get the reply we are expecting.
const isOldMessage =
this.expectedEchoMsgIds.get(attrName) !== msgId;
this._expectedEchoMsgIds.get(attrName) !== msgId;
if (isOldMessage) {
// Ignore an echo update that comes before our echo.
delete state[attrName];
} else {
// we got our echo confirmation, so stop looking for it
this.expectedEchoMsgIds.delete(attrName);
this._expectedEchoMsgIds.delete(attrName);
// Start accepting echo updates unless we plan to send out a new state soon
if (
this._msg_buffer !== null &&
Expand Down Expand Up @@ -456,7 +456,7 @@ export class WidgetModel extends Backbone.Model {
}

Object.keys(attrs).forEach((attrName: string) => {
this.attrsToUpdate.add(attrName);
this._attrsToUpdate.add(attrName);
});

const msgState = this.serialize(attrs);
Expand Down Expand Up @@ -499,10 +499,10 @@ export class WidgetModel extends Backbone.Model {
}
}
rememberLastUpdateFor(msgId: string) {
this.attrsToUpdate.forEach((attrName) => {
this.expectedEchoMsgIds.set(attrName, msgId);
this._attrsToUpdate.forEach((attrName) => {
this._expectedEchoMsgIds.set(attrName, msgId);
});
this.attrsToUpdate = new Set<string>();
this._attrsToUpdate = new Set<string>();
}

/**
Expand Down Expand Up @@ -538,6 +538,9 @@ export class WidgetModel extends Backbone.Model {

/**
* Send a sync message to the kernel.
*
* If a message is sent successfully, this returns the message ID of that
* message. Otherwise it returns an empty string
*/
send_sync_message(state: JSONObject, callbacks: any = {}): string {
if (!this.comm) {
Expand Down Expand Up @@ -680,9 +683,9 @@ export class WidgetModel extends Backbone.Model {
// keep track of the msg id for each attr for updates we send out so
// that we can ignore old messages that we send in order to avoid
// 'drunken' sliders going back and forward
private expectedEchoMsgIds: Map<string, string>;
private _expectedEchoMsgIds: Map<string, string>;
// because we don't know the attrs in _handle_status, we keep track of what we will send
private attrsToUpdate: Set<string>;
private _attrsToUpdate: Set<string>;
}

export class DOMWidgetModel extends WidgetModel {
Expand Down
4 changes: 3 additions & 1 deletion packages/base/test/src/dummy-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,15 @@ export class MockComm implements widgets.IClassicComm {
return '';
}
send(): string {
return '';
this._msgid += 1;
return this._msgid.toString();
}
comm_id: string;
target_name: string;
_on_msg: Function | null = null;
_on_open: Function | null = null;
_on_close: Function | null = null;
_msgid = 0;
}

const typesToArray: { [key: string]: any } = {
Expand Down
142 changes: 70 additions & 72 deletions packages/base/test/src/widget_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,76 @@ describe('WidgetModel', function () {
});
expect(customEventCallback).to.be.calledOnce;
});

it('ignores echo_update messages when there is an expected echo_update', async function () {
const send = sinon.spy(this.widget, 'send_sync_message');
// Set a value, generating an update message, get the message id from the comm?
this.widget.set('a', 'original value');
this.widget.save_changes();

// Get the msg id
const msgId = send.returnValues[0];

// Inject a echo_update message from another client
await this.widget._handle_comm_msg({
parent_header: {
msg_id: 'other-client',
},
content: {
data: {
method: 'echo_update',
state: { a: 'other client update 1' },
},
},
});

expect(this.widget.get('a')).to.equal('original value');

// Process a kernel update message, which should set the value
await this.widget._handle_comm_msg({
parent_header: {
msg_id: 'from-kernel',
},
content: {
data: {
method: 'update',
state: { a: 'kernel update' },
},
},
});

expect(this.widget.get('a')).to.equal('kernel update');

// Inject an echo_update message from us, resetting our value
await this.widget._handle_comm_msg({
parent_header: {
msg_id: msgId,
},
content: {
data: {
method: 'echo_update',
state: { a: 'original value' },
},
},
});

expect(this.widget.get('a')).to.equal('original value');

// Inject another echo_update message from another client, which also updates us
await this.widget._handle_comm_msg({
parent_header: {
msg_id: 'other-client-2',
},
content: {
data: {
method: 'echo_update',
state: { a: 'other client update 2' },
},
},
});

expect(this.widget.get('a')).to.equal('other client update 2');
});
});

describe('_deserialize_state', function () {
Expand Down Expand Up @@ -430,78 +500,6 @@ describe('WidgetModel', function () {
});
});

describe('_handle_comm_msg', function () {
beforeEach(async function () {
await this.setup();
});

it('handles update messages', async function () {
const deserialize = this.widget.constructor._deserialize_state;
const setState = sinon.spy(this.widget, 'set_state');
const state_change = this.widget._handle_comm_msg({
content: {
data: {
method: 'update',
state: { a: 5 },
},
},
});
expect(this.widget.state_change).to.equal(state_change);
await state_change;
expect(deserialize).to.be.calledOnce;
expect(setState).to.be.calledOnce;
expect(deserialize).to.be.calledBefore(setState);
expect(this.widget.get('a')).to.equal(5);
});

it('updates handle various types of binary buffers', async function () {
const buffer1 = new Uint8Array([1, 2, 3]);
const buffer2 = new Float64Array([2.3, 6.4]);
const buffer3 = new Int16Array([10, 20, 30]);
await this.widget._handle_comm_msg({
content: {
data: {
method: 'update',
state: { a: 5, c: ['start', null, {}] },
buffer_paths: [['b'], ['c', 1], ['c', 2, 'd']],
},
},
buffers: [buffer1, buffer2.buffer, new DataView(buffer3.buffer)],
});
expect(this.widget.get('a')).to.equal(5);
expect(this.widget.get('b')).to.deep.equal(new DataView(buffer1.buffer));
expect(this.widget.get('c')).to.deep.equal([
'start',
new DataView(buffer2.buffer),
{ d: new DataView(buffer3.buffer) },
]);
});

it('handles custom deserialization', async function () {
await this.widget._handle_comm_msg({
content: {
data: {
method: 'update',
state: { halve: 10, times3: 4 },
},
},
});
expect(this.widget.get('halve')).to.equal(5);
expect(this.widget.get('times3')).to.equal(12);
});

it('handles custom messages', function () {
const customEventCallback = sinon.spy();
this.widget.on('msg:custom', customEventCallback);
this.widget._handle_comm_msg({
content: {
data: { method: 'custom' },
},
});
expect(customEventCallback).to.be.calledOnce;
});
});

describe('set_state', function () {
beforeEach(async function () {
await this.setup();
Expand Down
Loading

0 comments on commit ac7c4b9

Please sign in to comment.