Why is this change needed?

As the comment says, this should be a no-op. The underlying socket is not supposed to deliver a read event after a close event, but TLS would try to do so (while racing to destroy it first). Looking back at the commit history, it was added as a hack to work around this issue, but didn't really fix it, as evidenced by flaky CI.

If there is buffered data (for example if there is buffered data and the socket is destroyed) shouldn't it be read? We do something similar in ws to ensure that all received data is actually used and delivered to the user.

There is no data buffered here, that wouldn't make any sense as the http protocol doesn't allow for more http data to be included after the http message is ended.

How do you know that the http message ended? Isn't this only a listener for the 'close' event of the socket? That 'close' event can be emitted in the middle of a HTTP request/response.

I am not sure what you mean by calling it "only" the 'close' event. The 'close' event is emitted to signal the end of an HTTP request/response stream. Ergo it is logically incompatible for it to happen in the middle.

Ref: https://nodejs.org/api/stream.html#stream_event_close_1 "This event indicates that no more events will be emitted, and no further computation will occur."

This 'close' event is emitted by the {net,tls}.Socket and it can be emitted when there is buffered data in its readable queue. For example if the socket is paused and it gets destroyed.

The documentation for the 'close' event I linked there forbids calling the ondata callback here, or any other future callbacks, so the comment there must be wrong about that expectation. Note that the previous bug this was supposed to address wasn't even pulling through a final buffered chunk, but instead was tricking nodejs into thinking there was an error handler for events arriving in the next event loop tick, which mostly hides the underlying bug when doing local testing on a loop-back interface on the linux kernel, so this comment was also incorrect in its description of why it is added.

If you got here by calling 'destroy', then calling .read() here afterwards introduces a multi-process data race, and no number of .read() calls is sufficient to recover from that error.

@vtjnash this is basically what I mean:

const assert = require('assert');
const net = require('net');

const chunk = Buffer.alloc(16 * 1024);

const server = net.createServer(function (socket) {
  socket.write(chunk);
});

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

  socket.on('close', function () {
    const data = socket.read();

    assert.strictEqual(data.length, 16 * 1024);
    server.close();
  });

  setTimeout(function () {
    assert.strictEqual(socket.readableLength, 16 * 1024);
    assert.ok(socket.isPaused);
    socket.destroy();
  }, 100);
});

without that socket.read() after the 'close' event that data that was actually received might never be used.

I am not sure what your example is supposed to show, as you are using the return value of socket.read to "prove" that a callback happens elsewhere which will not happen. If I modify your example to match the description of behavior in the comment, we can see that the behavior asserted in the comment is never what actually happens.

This logs "close", and then will never call "data" (because the stream is paused). If the comment was correct, it should then also print "data 16384" afterwards.

const assert = require('assert');
const net = require('net');

const chunk = Buffer.alloc(16 * 1024);

const server = net.createServer(function (socket) {
  socket.write(chunk);
});

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

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

  socket.on('end', function () {
    console.log('ended');
  });

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

  socket.on('data', function (data) {
    console.log('data ' + data.length);
    assert.strictEqual(data.length, 16 * 1024);
  });

  socket.pause();

  setTimeout(function () {
    assert.strictEqual(socket.readableLength, 16 * 1024);
    assert.ok(socket.isPaused);
    socket.destroy();
  }, 100);
});

Eww, okay, I dropped the socket.read() call, and then that printed "data 16384", in contradiction to the documentation which states that "no more events will be emitted, and no further computation will occur.

But back to the actual context here: why would http ever be calling .pause and .destroy on the underlying socket in the middle of reading a message?

@vtjnash

const assert = require('assert');
const net = require('net');

const chunk = Buffer.alloc(16 * 1024);

const server = net.createServer(function (socket) {
  socket.write(chunk);
});

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

  socket.on('close', function () {
    socket.read();
  });

  setTimeout(function () {
    assert.strictEqual(socket.readableLength, 16 * 1024);
    socket.pause();

    socket.on('data', function (chunk) {
      console.log('data');
      assert.strictEqual(chunk.length, 16 * 1024);
      server.close();
    })

    socket.destroy();
  }, 100);
});

I'm only calling socket.pause() for simplicity here. It could be done implicitly for backpressure handling when the HTTP response is piped to a slower stream. Without socket.read() the 'data' event is not emitted and the data is simply lost. Again, we use the same pattern in ws.

But back to the actual context here: why would http ever be calling .pause and .destroy on the underlying socket in the middle of reading a message?

Because the HTTP request and response are streams that can be piped around and as streams they might be destroyed.

If you bypass the HTTP protocol to call 'destroy' in the middle of reading the response, isn't it expected that possible unread data to be lost? The documentation for 'destroy' is not entirely clear on what it does precisely to buffered data (just that it should "release any internal resources" and should truncate the stream by ignoring any future data received).

But it sounds like this situation would be bypassing the internal requirements of the http module, to explicitly violate the http protocol. That doesn't sound like something this code should have been attempting to partially handle.

Looking at 'ws' for '.destroy' calls, most of them happen after 'end', which would be unaffected by this removal (in addition to not being http, so really not relevant to this code also). Or in places where it wants to immediately stop further processing on the stream (e.g. auth failures). The only place there that I see an issue with the use of 'destroy' is in a bit of example code, which I see you merged recently (websockets/ws#1798) which tries to do a write immediate before a destroy call, which the documentation for 'writable.destroy' warns against (see my comment there), but that doesn't seem to be particularly relevant here to our discussion of the handling of 'readable.destroy' either.

I don't know. In ws we want all received bytes to be parsed, including the ones kept in the socket buffer because they have actually been read and reached the peer/application.

If you are confident that this will not cause issues in practice because userland modules are not relying on current behavior, then I'm fine with it.

This change does not affect ws luckily. I've only mentioned it because the same pattern used here (to read buffered data after 'close') is also used by ws as per #36111 (comment).

Did you have an example for why you added those lines to ws? I see only the commit where you added it:
websockets/ws@6046a28. Note that the documentation for 'error' also indicates that this call to .read should not trigger any callbacks to occur ("no further events other than 'close' should be emitted"), so by adding that comment and code there, it seems like you may be causing ws to violate the stream.Readable interface.

It also seems like you had previously changed it to explicitly have the opposite behavior in websockets/ws@5d8ab0e, but was changed back later during a general code refactoring? From the comment description, it appears to me to say that code was intending to be be part of the 'error' handler (before that handler called destroy), and should be triggering the 'readable' callback, not the 'read' callback.

But why would the underlying socket be doing an active 'read', thus causing it to encounter an 'error', while data was already pending in the buffer? That seems like it would be disregarding of flow control, so wouldn't actually happen in a correctly implemented stream.

AFAIK calling readable.read() is the only way to pull out buffered data from a readable stream after it is destroyed and in my opinion there must be a way to do that. The fact that doing so causes an additional 'data' event is a side effect that is actually very useful.

Did you have an example for why you added those lines to ws?

See #36111 (comment) and #36111 (comment). The user can call ws.terminate() and destroy the socket. When that happens there might be buffered data in the socket. That data might contain many vaid WebSocket frames that we do not want to drop.

websockets/ws@5d8ab0e is still in place, see https://github.com/websockets/ws/blob/8.0.0/lib/websocket.js#L946. Any data after the close frame must be discarded per specification.

Anyway this is not about ws. ws is not affected by this change. This is about discarding data that was previously not discarded.

I haven't fully followed this discussion but #39639 might be of relevance.