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.

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

Do not waste time compressing when socket is closed #1464

Merged
merged 13 commits into from Nov 6, 2018

Conversation

@Evertras
Copy link
Contributor

@Evertras Evertras commented Oct 23, 2018

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.

lib/sender.js Outdated

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

@lpinca lpinca Oct 23, 2018

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

@Evertras Evertras Oct 23, 2018

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

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

@Evertras Evertras Oct 25, 2018

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

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.

lib/sender.js Outdated

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

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

@lpinca lpinca Oct 23, 2018

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

Copy link
Contributor Author

@Evertras Evertras Oct 23, 2018

Ah, thought it was required. Adding a check.

lib/sender.js Outdated Show resolved Hide resolved
@@ -42,7 +42,8 @@ describe('Sender', function () {
write: (data) => {
assert.strictEqual(data[0] & 0x40, 0x40);
if (++count === 3) done();
}
},
on: () => {}
Copy link
Member

@lpinca lpinca Oct 23, 2018

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

@Evertras Evertras Oct 23, 2018

Sounds good, will switch them over.

@lpinca
Copy link
Member

@lpinca lpinca commented Oct 23, 2018

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

  1. 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.
  2. 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.

@Evertras
Copy link
Contributor Author

@Evertras Evertras commented Oct 23, 2018

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.

@Evertras
Copy link
Contributor Author

@Evertras Evertras commented Oct 25, 2018

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?

lib/sender.js Outdated
@@ -27,6 +27,24 @@ class Sender {
this._bufferedBytes = 0;
this._deflating = false;
this._queue = [];

this._socket.prependOnceListener('close', () => {
Copy link
Member

@lpinca lpinca Oct 25, 2018

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.

Copy link
Contributor Author

@Evertras Evertras Oct 25, 2018

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

Copy link
Member

@lpinca lpinca Oct 25, 2018

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.

Copy link
Contributor Author

@Evertras Evertras Oct 25, 2018

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

Copy link
Contributor Author

@Evertras Evertras Oct 25, 2018

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
Copy link
Member

@lpinca lpinca commented Oct 25, 2018

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 Outdated Show resolved Hide resolved
Co-Authored-By: Evertras <bfullj@gmail.com>
@lpinca
Copy link
Member

@lpinca lpinca commented Oct 25, 2018

@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
Copy link
Contributor Author

@Evertras Evertras commented Oct 26, 2018

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
Copy link
Member

@lpinca lpinca commented Oct 26, 2018

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
Copy link
Member

@lpinca lpinca commented Oct 26, 2018

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
Copy link
Contributor Author

@Evertras Evertras commented Oct 27, 2018

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
Copy link
Member

@lpinca lpinca commented Nov 3, 2018

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

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
Copy link
Member

@lpinca lpinca commented Nov 6, 2018

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

@lpinca lpinca merged commit 7d51fb9 into websockets:master Nov 6, 2018
3 checks passed
@lpinca
Copy link
Member

@lpinca lpinca commented Nov 6, 2018

Thank you!

lpinca added a commit that referenced this issue Nov 10, 2018
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 issue Nov 10, 2018
Do not invoke callbacks when clearing the send queue due to premature
socket closure.

Refs: #1464 (comment)
Fixes #1226
@Evertras
Copy link
Contributor Author

@Evertras Evertras commented Nov 10, 2018

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

lpinca added a commit that referenced this issue Nov 14, 2018
Do not invoke callbacks when clearing the send queue due to premature
socket closure.

Refs: #1464 (comment)
Fixes #1226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants