Do not waste time compressing when socket is closed #1464

Evertras · 2018-10-23T09:00:08Z

Addresses #1226 - I agree with @lpinca 's approach in general. This has been tested with the offending code listed in the issue and it correctly short circuits while still providing a relevant error to each pending request. I've also added a unit test that fails without the added code as it tries to write/compress too much.

Not sure how I feel about having to add the extra on: () => {} to the Sender unit tests for the socket object, but these are mocks anyway. Open to suggestions if this isn't preferred.

lpinca · 2018-10-23T14:42:00Z

lib/sender.js

+
+    this._socket.on('close', () => {
+      const err = new Error(
+        `WebSocket is not open: readyState ${this._socket.readyState} `


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

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.

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

Set the ready state to CLOSING

Use it to make the error message consistent.

What do you think?

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.

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.

lpinca · 2018-10-23T14:46:31Z

lib/sender.js

+
+        this._bufferedBytes -= params[1].length;
+
+        params[params.length - 1](err);


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

Ah, thought it was required. Adding a check.

lib/sender.js

lpinca · 2018-10-23T15:09:41Z

test/sender.test.js

@@ -42,7 +42,8 @@ describe('Sender', function () {
        write: (data) => {
          assert.strictEqual(data[0] & 0x40, 0x40);
          if (++count === 3) done();
-        }
+        },
+        on: () => {}


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 });

Sounds good, will switch them over.

lpinca · 2018-10-23T16:10:00Z

Thank you for the PR. Some thoughts in addition to the comments:

Each callback should ideally be called before the 'close' event is emitted on the WebSocket. The current approach relies on the fact that the 'close' event listener is added to the net.Socket before the 'close' listener that actually emit 'close' on the WebSocket. This is good and should work but there might be a chunk that is already being compressed when the 'close' event is emitted on the net.Socket and this is not caught because it is no longer in the queue. As a result the callback for this message could be invoked after the 'close' event is emitted on the WebSocket and after all other callbacks that are short circuited.
Each callback should find the ready state to CLOSING. If I'm not wrong when the queue is cleared the WebSocket ready state is still OPEN so the user might be surprised to get the error and have the ready state set to OPEN.

The second point should be easy to fix, it should be sufficient to set the ready state to CLOSING in the 'close' listener added in the Sender before clearing the queue.

The first point is harder and I'm not sure how to fix that.

Co-Authored-By: Evertras <bfullj@gmail.com>

Evertras · 2018-10-23T22:37:46Z

~~For the second point, my testing is showing the ready state as "closed". Is "closing" a better state here?~~ I see what you mean now. Fixing.

…ected state

Evertras · 2018-10-25T06:27:40Z

I've tested locally with a modified version of the script in the issue and added a unit test to make sure that the state is indeed CLOSING in the callbacks when they're in the queue. I also changed the once to a prepend version to make it explicit that this needs to run first. I'm not sure how to reproduce the hypothetical you listed, is there an example you could provide?

lpinca · 2018-10-25T06:29:18Z

lib/sender.js

@@ -27,6 +27,24 @@ class Sender {
    this._bufferedBytes = 0;
    this._deflating = false;
    this._queue = [];
+
+    this._socket.prependOnceListener('close', () => {


Do we need prependOnceListener()? Or is it used only to ensure that the listener is added before the one added in websocket.js? In the latter case I would just add a comment in WebSocket#setSocket() to specify that the Sender must be instantiated before adding the 'close' listener to the net.Socket.

Turns out we do actually need it. I was testing a different ready state in my initial pass.

Hmm ok, that's strange as this is added before the other anyway. I'm not a fan of prependOnceListener() because it will add the listener before all other listeners including the ones added by Node.js itself.

Let me poke it again, I may end up blaming jet lag. :D

Yep, going to blame jet lag. Sorry for the confusion. We do not need prepend, I'll switch it over to the comment instead. The issue was I was switching prepend to once in the Sender but not in the MockSocket for the test.

lpinca · 2018-10-25T06:36:55Z

I'm not sure how to reproduce the hypothetical you listed, is there an example you could provide?

I'll try to create a test case when I get a chance. Thank you.

lib/sender.js

Co-Authored-By: Evertras <bfullj@gmail.com>

lpinca · 2018-10-25T20:18:56Z

@Evertras here is the test case

const WebSocket = require('ws');
const EventEmitter = require('events');
const { randomBytes } = require('crypto');

const ee = new EventEmitter();

const wss = new WebSocket.Server({
  perMessageDeflate: { threshold: 0 },
  port: 0
}, function () {
  const ws = new WebSocket(`ws://localhost:${wss.address().port}`, {
    perMessageDeflate: { threshold: 0 }
  });

  ws.on('open', function () {
    const pmd = ws._extensions['permessage-deflate'];
    const compress = pmd.compress;

    // Monkey patch compress to show that deflate ends after the `'close'` event
    // is emitted.
    pmd.compress = function (data, fin, callback) {
      compress.call(pmd, data, fin, function (err, buf) {
        console.log('deflate done');
        callback(err, buf);
      });
    };

    ws.send(randomBytes(1024 * 1024), function (err) {
      if (err) console.log(err);
    });

    // Make the other peer destory the socket while data is being compressed.
    ee.emit('destroy');
  });

  ws.on('close', function () {
    console.log('close');
  });
});

wss.on('connection', function (ws) {
  ee.on('destroy', function () {
    ws.terminate();
  });
});

Evertras · 2018-10-26T08:39:46Z

I see. The deflate has already started by the time the close event from the socket is received, so by the time we're even aware that the socket has closed then zlib has already started to do its work. The closest I could get to an interruption is in this callback and at that point all the expensive work is done.

I'm not sure if it's even possible to completely remove this scenario. Once it's passed to zlib, some compression may take place while the socket is closing since it's all batched up in the flush call. I'm also not sure if it's worth trying to chase.

lpinca · 2018-10-26T08:58:18Z

Yes, I think there is not much we can do about it, we could check if the socket is closed here so at least we avoid framing that chunk but I'm not sure if it's worth the effort, it's really and edge case.

The only thing that bugs me is that when this happens the order of callbacks is no longer respected. The callback of the chunk being compressed is called after all other callbacks that are short circuited and that is why in the original issue I thought it was better to clear the send queue here but if we do that, callbacks are invoked after the 'close' event is emitted.

lpinca · 2018-10-26T09:36:10Z

It could be addressed by refactoring the already complex close procedure, for example by emitting the 'close' event only when both the Receiver and Sender complete their processing.

That said, I think this PR is already a big improvement and could land as is, just give me a few days to review it again (sorry I'm kinda busy).

Sometimes I think adding permessage-deflate was a mistake, the amount of complexity it added is huge.

Thank you.

Evertras · 2018-10-27T01:53:01Z

Agreed there are ways to improve it, but yeah, it'll be complicated. One step at a time I think. No rush on approval, thanks for all the input!

lpinca · 2018-11-03T10:54:52Z

@Evertras I was thinking that maybe we can simplify everything by simply clearing the queue here

ws/lib/sender.js

Line 335 in 7e061bc

options.readOnly = false;

(if the socket is no longer writable) without even calling the callbacks.

For example, consider this example:

const { createConnection, createServer } = require('net');
const { randomBytes } = require('crypto');
const { Writable } = require('stream');

function callback(err) {
  console.error(err);
}

const server = createServer();

server.on('connection', function (socket) {
  const chunk = randomBytes(1024);

  socket.on('error', function (err) {
    console.error(err);
  });

  while (socket.write(chunk)) {}

  // Write some more.
  for (var i = 0; i < 16; i++) {
    socket.write(chunk, callback);
  }
});

server.listen(function () {
  const socket = createConnection(this.address().port);

  socket.on('connect', function () {
    const writable = new Writable({
      write(chunk, encoding, callback) {
        setTimeout(function () {
          callback();
          socket.destroy();
        }, 100);
      }
    });

    socket.pipe(writable);
  });
});

queued callbacks are not called. My point is that even when not using permessage-delfate, there are already cases where callbacks are not called if the connection is not closed cleanly.

What do you think?

lpinca · 2018-11-06T06:47:39Z

I'll merge this in the meanwhile and think about #1464 (comment) a little more before taking any action.

lpinca · 2018-11-06T06:48:45Z

Thank you!

Do not invoke callbacks when clearing the send queue due to premature socket closure. Refs: #1464 (comment) Fixes #1226

Evertras · 2018-11-10T16:40:34Z

Thanks for the merge! I've been in zombie mode for the past week due to a nasty illness, sorry for not responding earlier.

Do not invoke callbacks when clearing the send queue due to premature socket closure. Refs: #1464 (comment) Fixes #1226

Do not invoke callbacks when clearing the send queue due to premature socket closure. Refs: websockets/ws#1464 (comment) Fixes #1226

Evertras added 2 commits October 23, 2018 01:53

Do not waste time trying to compress when socket is closed

d160344

Remove errant extra new line

b265319

lpinca reviewed Oct 23, 2018

View reviewed changes

lpinca and others added 3 commits October 23, 2018 14:49

Use once instead of on

d75800e

Co-Authored-By: Evertras <bfullj@gmail.com>

Update sender tests to use a simple mock socket

c936a4c

Check if callback is actually a function before calling it

d780571

Evertras added 2 commits October 24, 2018 22:46

Add tests to ensure erroring callbacks are in expected order with exp…

0444d5c

…ected state

Better test for CLOSING state on buffered compression attempts

e2f4342

lpinca reviewed Oct 25, 2018

View reviewed changes

Remove errant console log

edda00c

Evertras added 2 commits October 24, 2018 23:50

Switch back to once instead of prepend, add comment for clarity instead

88c5474

Fix error message for queued sender callbacks

35105d2

lpinca reviewed Oct 25, 2018

View reviewed changes

lib/sender.js Outdated Show resolved Hide resolved

Update lib/sender.js

3303832

Co-Authored-By: Evertras <bfullj@gmail.com>

lpinca added 2 commits October 29, 2018 09:01

Add the listener only when permessage-deflate is enabled

20e9d85

Fix nits

dc90638

lpinca merged commit 7d51fb9 into websockets:master Nov 6, 2018

lpinca added a commit that referenced this pull request Nov 10, 2018

[minor] Ignore callbacks when clearing the send queue

f0d092c

Do not invoke callbacks when clearing the send queue due to premature socket closure. Refs: #1464 (comment) Fixes #1226

lpinca mentioned this pull request Nov 10, 2018

Ignore callbacks when clearing the send queue #1471

Merged

lpinca added a commit that referenced this pull request Nov 10, 2018

[minor] Ignore callbacks when clearing the send queue

1ae7709

Do not invoke callbacks when clearing the send queue due to premature socket closure. Refs: #1464 (comment) Fixes #1226

lpinca added a commit that referenced this pull request Nov 14, 2018

[minor] Ignore callbacks when clearing the send queue (#1471)

f26fac8

Do not invoke callbacks when clearing the send queue due to premature socket closure. Refs: #1464 (comment) Fixes #1226

lpinca mentioned this pull request Jun 18, 2019

Type error on error closing connection #1591

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not waste time compressing when socket is closed #1464

Do not waste time compressing when socket is closed #1464

Evertras commented Oct 23, 2018

lpinca Oct 23, 2018

Evertras Oct 23, 2018

lpinca Oct 25, 2018 •

edited

Evertras Oct 25, 2018

lpinca Oct 25, 2018 •

edited

lpinca Oct 23, 2018

Evertras Oct 23, 2018

lpinca Oct 23, 2018

Evertras Oct 23, 2018

lpinca commented Oct 23, 2018 •

edited

Evertras commented Oct 23, 2018 •

edited

Evertras commented Oct 25, 2018

lpinca Oct 25, 2018

Evertras Oct 25, 2018

lpinca Oct 25, 2018

Evertras Oct 25, 2018

Evertras Oct 25, 2018

lpinca commented Oct 25, 2018

lpinca commented Oct 25, 2018

Evertras commented Oct 26, 2018

lpinca commented Oct 26, 2018 •

edited

lpinca commented Oct 26, 2018

Evertras commented Oct 27, 2018

lpinca commented Nov 3, 2018 •

edited

lpinca commented Nov 6, 2018

lpinca commented Nov 6, 2018

Evertras commented Nov 10, 2018


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

		params[params.length - 1](err);

Do not waste time compressing when socket is closed #1464

Do not waste time compressing when socket is closed #1464

Conversation

Evertras commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Oct 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Oct 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Oct 23, 2018 • edited

Evertras commented Oct 23, 2018 • edited

Evertras commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Oct 25, 2018

lpinca commented Oct 25, 2018

Evertras commented Oct 26, 2018

lpinca commented Oct 26, 2018 • edited

lpinca commented Oct 26, 2018

Evertras commented Oct 27, 2018

lpinca commented Nov 3, 2018 • edited

lpinca commented Nov 6, 2018

lpinca commented Nov 6, 2018

Evertras commented Nov 10, 2018

lpinca Oct 25, 2018 •

edited

lpinca Oct 25, 2018 •

edited

lpinca commented Oct 23, 2018 •

edited

Evertras commented Oct 23, 2018 •

edited

lpinca commented Oct 26, 2018 •

edited

lpinca commented Nov 3, 2018 •

edited