Repository navigation
TCP socket emitted 'end' event after 'error' event #6083
Description
- Version: observed on v0.12, v4, and v5. Did not observe on v0.10. (details below)
- Platform: observed on both illumos and OS X Mountain Lion, both 32-bit and 64-bit (details below)
- Subsystem: net
Although it's not technically documented, a lot of code assumes that once a stream emits an 'error' event, it will not subsequently emit 'data' or 'end' events. This is pretty much necessary, because otherwise it's impossible for a stream consumer to know when a stream has come to rest and will emit no more events. I've reproduced a case where Node reliably emits an 'end' event after an 'error' event, which violates the expectations of code that assumes a stream has come to rest after 'error'.
To be really precise about where I tested it:
Works as expected on v0.10 (perhaps only because the 'end' event is emitted first):
- illumos: v0.10.43 (both 32-bit and 64-bit)
- OS X Mountain Lion: v0.10.28 (64-bit)
Does not work as expected on v0.12 and later:
- illumos: v0.12.12, v4.4.0, v5.9.0 (both 32-bit and 64-bit)
- OS X Mountain Lion: v0.12.2, v4.4.2, v5.10.0 (64-bit)
Here's a test case that's commented with what it's doing. I've tried to simplify it as much as possible, but since it's a race condition, it's tricky to get the timing just right.
/*
* test-stream.js: demonstrates a case where a Node stream can see an 'end'
* event after an 'error' event.
*
* This test case works as follows:
*
* (1) Set up a TCP server socket and connect to it using a TCP client.
* Server: set up listeners for 'end' and 'error'.
* Client: set up listener for 'error'.
*
* (2) Client: write 65536 bytes of data.
*
* (3) Pause one second. Behind the scenes, Node will detect that the
* server's socket has become readable and read all 65536 bytes. These
* will be buffered in JavaScript.
*
* (4) Server: read 65535 bytes of data from the Socket. There will be
* one byte left buffered on the Socket in JavaScript.
* Client: destroy the socket. This will send a FIN to the server.
*
* (5) Asynchronously (via setImmediate):
* Server: read 1 byte of data from the Socket. This will trigger Node
* to read from the underlying socket again, where it will read 0
* bytes, signifying the end of the stream.
*
* Server: write data to the socket. Since the socket is now
* disconnected, eventually these writes will report EPIPE/SIGPIPE.
* This generally happens synchronously with respect to the write()
* call, but the error will be emitted asynchronously.
*
* (6) Asynchronously (via setImmediate):
* Server: read another byte from the socket. At this point, we're
* reading past end-of-stream, and Node will schedule an 'end' event to
* be emitted, but an 'error' event has already been scheduled as well,
* so we'll see 'error' and then 'end', which should be invalid.
*/
var mod_net = require('net');
var mod_os = require('os');
/* IP address and port used for this test case. */
var ip = '127.0.0.1';
var port = 16404;
/* We'll use this buffer as a chunk of data. */
var bufsz = 64 * 1024;
var buf;
/* State for this test */
var server; /* server's listening socket */
var ssock; /* server's connection socket */
var csock; /* client socket */
var end = false; /* server has seen "end" on its connection socket */
var error = false; /* server has seen "error" on its connection socket */
function main()
{
console.log('versions:',
process.version, process.arch, mod_os.platform());
buf = new Buffer(bufsz);
buf.fill(0);
/*
* (1) Set up client and server.
*/
server = mod_net.createServer({ 'allowHalfOpen': true });
server.on('connection', function (s) {
console.log('server: client connected');
ssock = s;
ssock.on('end', function () {
console.log('server: saw "end" on client socket');
if (error) {
console.log('reproduced issue!');
process.abort();
}
end = true;
});
ssock.on('error', function (err) {
console.log('server: saw "error" on client socket', err);
if (error || end) {
console.log('bailing out after server error');
process.exit(0);
}
// ssock.read(1);
error = true;
});
/*
* (2) Client writes data.
*/
csock.write(buf);
/*
* (3) Pause until the server sees that data.
*/
ssock.once('readable', triggerIssue);
});
server.listen(port, function () {
console.log('server: listening');
csock = mod_net.createConnection(port, ip);
csock.on('connect', function () {
console.log('client: connected');
});
csock.on('end', function () {
console.log('client: saw "end" on server socket');
});
});
}
function triggerIssue()
{
console.log('triggering issue by destroying client socket');
/*
* (4) Read _most_ of the data from the socket and have the client
* destroy the socket.
*/
ssock.read(bufsz - 1);
csock.destroy();
setImmediate(function () {
/*
* (5) Read 1 byte of data from the socket and write data to it.
*/
ssock.read(1);
ssock.write(buf);
ssock.write(buf);
setImmediate(function () {
/*
* (6) Read one more byte.
*/
ssock.read(1);
});
});
}
main();The detailed output for each test I ran is here:
https://gist.github.com/davepacheco/84d450d2c25f6212a99a984a8f089b4c
Activity
Trott commentedon Jul 7, 2017
@nodejs/streams Is this a bug? Documentation issue? Neither? Both?
mcollina commentedon Jul 7, 2017
I would need to look into it.
michaelnisi commentedon Jul 8, 2017
Isn’t this unspecified and depends on the individual stream?
mcollina commentedon Jul 10, 2017
As @michaelnisi noted, this is unspecified and depends on the individual stream. To the best of my knowledge, there is no mechanism/enforcement in
netto prevent this. ForWritableandDuplexstreams (including sockets), there is the guarantee that'error'will only be emitted once.Also,
'error'will be emitted before'end','close'and'finish'. The correct approach is to remove all the listeners when'error'happens.That being said, I cannot reproduce the issue on node v4.4.2 (the one which output was linked in the issue), 4.8, 6.11, 8, on Mac OS X 10.12. @davepacheco is this still an issue? If that's the case, can you update the script to reproduce it, or maybe send a PR with a failing test?
The script currently hang with:
davepacheco commentedon Jul 10, 2017
Is there documentation that stream consumers should remove their 'close', 'end', and 'finish' listeners when 'error' happens? It seems like that makes using streams robustly a lot more complex.
I'm not seeing this on OS X El Capitan (v10.11.6), but I think that just means something about the timing has changed, not that the underlying issue was OS-specific or went away.
mcollina commentedon Jul 10, 2017
@davepacheco No, I do not think there is. However, most streams respect this generic pattern. Given the code we have in 8, I do not see how that can happen, as we destroy the handle synchronously in case of an internal error. I would be happy to take a look if you can reproduce anyhow.
The implementations in core should not have the problem you are describing, so it is a bug in that case. It might even be OSS specific, and maybe in libuv.
apapirovski commentedon Apr 12, 2018
There has been no movement on this in nearly a year and no reports of it happening again. I'm going to close it out but if any new info has surfaced or you believe this should remain open, please feel free to do so.
davepacheco commentedon Apr 16, 2018
Has anybody re-run the test program to see if it's still happening?
mcollina commentedon Apr 17, 2018
@davepacheco The situation has changed. The script hangs:
Or I'm getting a nice abort:
This is also confirmed on master.
mcollina commentedon Apr 17, 2018
Here is some output from llnode:
Do you know why it's missing the JS stacks? It's taken from master.
9 remaining items
stream: prevent 'end' to be emitted after 'error'
ORESoftware commentedon Mar 30, 2019
Sorry to bomb this thread but I am looking for info regarding whether a tcp connection can recover after an 'error' event is emitted. I am guessing not, but I am not really sure. Currently, if 'error' is emitted I call .end() on the current connection and try to open a entirely new one.
mcollina commentedon Mar 30, 2019
Unless you are emitting error yourself, you can safely assume that the connection is gone. You don’t need to call .end().
If you want to be safe, call .destroy()
ORESoftware commentedon Apr 1, 2019
@mcollina thanks appreciated
BridgeAR commentedon Jan 2, 2020
@ronag do you know if this is still an issue or if this is already fixed?
ronag commentedon Jan 2, 2020
@BridgeAR: It still seems to be an issue. As far as I can tell, the fix was reverted and the follow up was closed... I haven't followed the entire history and rationale of this.
@mcollina mentioned the following #20571 (comment)
But as far as I can see from master, this is still a problem, i.e.
Readablecan emit'end'after'error'.I wouldn't mind to pick up on this but would be useful if those involved so far could shime in? @mafintosh @mcollina
stream: do not emit 'end' after 'error'
ronag commentedon Jan 6, 2020
@BridgeAR: Now I think this is fixed :)
celesteking commentedon Mar 4, 2021
benjamingr commentedon Mar 4, 2021
@celesteking this sort of comment is entirely unacceptable in the Node.js org. Please self-moderate and avoid these sort of comments in the future. Please consider what exactly you're trying to accomplish here and how these comments reflect on you and others in the org.
stream: prevent 'end' to be emitted after 'error'