net: aborted socket incorrectly ends gracefully

[ronag]

The following test will fail since the client socket will emit 'end' and no ECONNRESET, even though the server called destroy() instead of end().

Platform: MacOS

'use strict';
const common = require('../common');
const net = require('net');
const assert = require('assert');

const server = net.createServer((socket) => {
  socket.resume();
  socket.on('close', common.mustCall(() => {
    server.close();
  }));
  socket.on('end', common.mustNotCall());
  socket.on('error', common.mustCall((err) => {
    assert.strictEqual(err.code, 'ECONNRESET');
  }));
});
server.listen(0, () => {
  const req = net.connect(server.address().port);
  req.resume();
  req.setTimeout(10, function() {
    req.destroy();
  });
});

[ronag]

@addaleax Maybe has an idea?

I notice there is no nodejs/net team. Who more is usually a good ping on net issues?

[santigimeno]

@ronag sorry, I'm not understanding why this is an issue. IIUIC, if you're not writing any data while the socket is closing with destroy() (close() syscall), you're not going to receive the ECONNRESET error.

[ronag]

I am not very knowledgeable on TCP, but I guess in the case of destroy() (unlike end() no FIN pack should be sent and the other side of the connection should detect non graceful disconnect (or RST) as ECONNRESET?

I'm not sure if there is such a thing as graceful and non graceful disconnect in tcp and how and whether that applies to node sockets, so please take this issue with a grain of salt.

[sam-github]

@ronag Destroy is entirely a node thing, it means "stop reading, stop writing, close socket". .end() means "write all queued data, write FIN, then close socket" (unless half open is allowed, then it means "write all queued data, write FIN (using SHUTDOWN), then read data until FIN or error, then close").

destroy doesn't mean "send a TCP error indication to the peer".

[santigimeno]

afaik, destroy() ends up calling uv_close() that performs a close() syscall performing the normal tcp disconnection procedure (sending the FIN packet) and closing both directions of the stream. end() on the other hand, calls shutdown(, SHUT_WR) syscall that sends also the FIN packet but only closing the writing side of the connection and waiting to receive the remaining data from the other peer before completely closing the connection.

[addaleax]

I was under the impression that destroy() would end up sending RST as part of the close() call. But if close() is called and no RST is sent/received, that’s unexpected but acceptable behaviour on the OS side, I guess?

[santigimeno]

To send the RST there's the uv_tcp_close_reset() method, but afaict it's not being used by node.

[sam-github]

.destroy() just closes while the node.js streams are not done, it doesn't specifically send a RST. It should be understood as "discard data queued in the stream; .end()".

destroy might appear to send RST occaisonally, because TCP close() can cause a RST in some very specific circumstances, such as more data is received from the other side after the socket was closed, or there is data received in this host's TCP buffers and not yet read by uv, but some of those situations could occur with .end() as well, they aren't .destroy() specific.

[ronag]

Just to clarify. Are we happy with the current behavior and what is the preferred behavior in a perfect world? For me destroy() before end() means ungraceful destruction (i.e. ERR_PREMATURE_CLOSE) which preferably should be propagated to the other side? to avoid a situation where the other side everything is done and ok.

[sam-github]

Yes, we are OK with current situation, though not with the docs!

Your expectations are based on assumptions about how TCP works (or desires for how it could work?) that don't reflect how it actually works, sorry!

Protocols on top of TCP are responsible for determining whether data was completely exchanged. HTTP does this, TLS does this, etc.

[ronag]

Yes, we are OK with current situation, though not with the docs!

I'm good with this. Thanks for clarifying!

[ronag]

though not with the docs!

@sam-github: How would you like to improve the docs?

[sam-github]

The docs have to make clear that destroy() rips node.js streams apart, but that they do not explicitly cause TCP stream reset (RST).

It should clarify what the defined behaviour is aftwards wrt. to read/write (and their callbacks), error events, and also enumerate the undefined behaviour. @ronag you likely have the best sense of that, as you seem to be going through and finding all the inconsistencies.

That would be enough to be clear for people familiar with TCP networking, but for many Node.js users, they are not clear on the bi-directional nature of TCP streams, and what they can expect of FIN and RST. This is a recurring tension in Node.js docs... people not familiar with underlying unix FS semantics are not likely to fully understand the fs docs... but giving a tutorial on fundamental programming concepts is difficult in Node.js API docs. Similar issues in TLS, net, HTTP, etc. I'm not sure what the solution is for that.

[ronag]

@sam-github Thanks. I'll try to draft something based on that.

The docs have to make clear that destroy() rips node.js streams apart, but that they do not explicitly cause TCP stream reset (RST).

Will add something along these lines.

It should clarify what the defined behaviour is aftwards wrt. to read/write (and their callbacks), error events, and also enumerate the undefined behaviour. @ronag you likely have the best sense of that, as you seem to be going through and finding all the inconsistencies.

This should be part of the streams docs. Work in progress.