Skip to content
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

Jump to bottom

Do not waste time compressing when socket is closed #1464

Merged
merged 13 commits into from Nov 6, 2018
Merged

Do not waste time compressing when socket is closed #1464

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d160344
Do not waste time trying to compress when socket is closed
Evertras Oct 23, 2018
b265319
Remove errant extra new line
Evertras Oct 23, 2018
d75800e
Use once instead of on
lpinca Oct 23, 2018
c936a4c
Update sender tests to use a simple mock socket
Oct 23, 2018
d780571
Check if callback is actually a function before calling it
Oct 23, 2018
0444d5c
Add tests to ensure erroring callbacks are in expected order with exp…
Evertras Oct 25, 2018
e2f4342
Better test for CLOSING state on buffered compression attempts
Evertras Oct 25, 2018
edda00c
Remove errant console log
Evertras Oct 25, 2018
88c5474
Switch back to once instead of prepend, add comment for clarity instead
Evertras Oct 25, 2018
35105d2
Fix error message for queued sender callbacks
Evertras Oct 25, 2018
3303832
Update lib/sender.js
lpinca Oct 25, 2018
20e9d85
Add the listener only when permessage-deflate is enabled
lpinca Oct 29, 2018
dc90638
Fix nits
lpinca Oct 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions lib/sender.js
View file
Open in desktop
Expand Up @@ -27,6 +27,20 @@ class Sender {
this._bufferedBytes = 0;
this._deflating = false;
this._queue = [];

this._socket.on('close', () => {
Evertras marked this conversation as resolved.
Show resolved Hide resolved
const err = new Error(
`WebSocket is not open: readyState ${this._socket.readyState} `
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit misleading because readyState here is the ready state of the net.Socket and not the ready state of the WebSocket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Would it be better to have a simple generic message here? The original intent was to provide consistency with what the user would get as an error regardless of whether their send attempt was queued or not, but since it was already inconsistent a generic message wouldn't break anything here.

Copy link
Member

@lpinca lpinca Oct 25, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can pass the Sender a reference to the WebSocket so we can:

  1. Set the ready state to CLOSING
  2. Use it to make the error message consistent.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to avoid that because I don't like how it hard links Sender to WebSocket. A Sender doesn't need a WebSocket to function.

Copy link
Member

@lpinca lpinca Oct 25, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, the same is also true for the Receiver but we add a WebSocket reference to it in order to use it in the event listeners.

);

while (this._queue.length) {
const params = this._queue.shift();

this._bufferedBytes -= params[1].length;

params[params.length - 1](err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback is optional so it's possible that the latest element is not a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thought it was required. Adding a check.

}
});
}

/**
Expand Down
78 changes: 68 additions & 10 deletions test/sender.test.js
View file
Open in desktop
Expand Up @@ -42,7 +42,8 @@ describe('Sender', function () {
write: (data) => {
assert.strictEqual(data[0] & 0x40, 0x40);
if (++count === 3) done();
}
},
on: () => {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid adding this every time, we can use something like this

class FakeSocket {
  constructor({ write, on } = {}) {
    if (write) this.write = write;
    if (on) this.on = on;
  }

  write() {}
  on() {}
}

and use it like

const socket = new FakeSocket({
  write: (data) => {
    assert.strictEqual(data[0] & 0x40, 0x40);
    if (++count === 3) done();
  }
});
const sender = new Sender(socket, { 'permessage-deflate': perMessageDeflate });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, will switch them over.

}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -57,13 +58,62 @@ describe('Sender', function () {
sender.send('hi', options);
});

it('does not attempt to compress enqueued messages after socket closes', function (done) {
const perMessageDeflate = new PerMessageDeflate({ threshold: 0 });
const numMessages = 1000;
let numWritten = 0;
const sender = new Sender({
write: (data) => {
assert.strictEqual(data[0] & 0x40, 0x40);
if (++numWritten > 1) done(new Error('Too many attempted writes'));
},
on: (ev, cb) => {
if (ev === 'close') {
process.nextTick(cb);
}
}
}, {
'permessage-deflate': perMessageDeflate
});

let numCompressed = 0;

perMessageDeflate.compress = (data, fin, cb) => {
if (++numCompressed > 1) {
done(new Error('Compressed too many times'));
}

setTimeout(() => cb(null, data), 1);
};

perMessageDeflate.accept([{}]);

const options = { compress: true, fin: false };
const array = new Uint8Array([0x68, 0x69]);

sender.send(array.buffer, options, () => {});
sender.send(array, options, () => {});

let numErrors = 0;
for (let i = 0; i < numMessages; ++i) {
sender.send('hi', options, (err) => {
if (!err) return;

if (++numErrors === numMessages) {
done();
}
});
}
});

it('does not compress data for small payloads', function (done) {
const perMessageDeflate = new PerMessageDeflate();
const sender = new Sender({
write: (data) => {
assert.notStrictEqual(data[0] & 0x40, 0x40);
done();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -86,7 +136,8 @@ describe('Sender', function () {
assert.strictEqual(fragments[1][0] & 0x40, 0x00);
assert.strictEqual(fragments[1].length, 6);
done();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -110,7 +161,8 @@ describe('Sender', function () {
assert.strictEqual(fragments[1][0] & 0x40, 0x00);
assert.strictEqual(fragments[1].length, 5);
done();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -134,7 +186,8 @@ describe('Sender', function () {
assert.strictEqual(fragments[1][0] & 0x40, 0x00);
assert.strictEqual(fragments[1].length, 8);
done();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -158,7 +211,8 @@ describe('Sender', function () {
assert.strictEqual(fragments[1][0] & 0x40, 0x00);
assert.strictEqual(fragments[1].length, 3);
done();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -175,7 +229,8 @@ describe('Sender', function () {
const sender = new Sender({
write: () => {
if (++count > 1e4) done();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -202,7 +257,8 @@ describe('Sender', function () {

assert.ok(data.equals(Buffer.from([0x89, 0x02, 0x68, 0x69])));
if (count === 4) done();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -228,7 +284,8 @@ describe('Sender', function () {

assert.ok(data.equals(Buffer.from([0x8a, 0x02, 0x68, 0x69])));
if (count === 4) done();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand All @@ -253,7 +310,8 @@ describe('Sender', function () {
write: (data, cb) => {
count++;
if (cb) cb();
}
},
on: () => {}
}, {
'permessage-deflate': perMessageDeflate
});
Expand Down