Skip to content

Follow up on echo updates #3407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 8, 2022
Merged

Follow up on echo updates #3407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6cccc9c
Add tests for setting JUPYTER_WIDGETS_ECHO to False
jasongrout Mar 4, 2022
ca5e3e5
Add doc message to send_sync_message
jasongrout Mar 4, 2022
53c98a3
Add a simple test for the echo_update messages
jasongrout Mar 4, 2022
686ec04
Rename echo message private attributes to follow the underscore conve…
jasongrout Mar 4, 2022
133b78c
Lint
jasongrout Mar 4, 2022
94ffe26
Proposal to better keep tests in sync
vidartf Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions packages/base/src/widget.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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