readable event not emitted after net.Socket reconnects

[morkai]

• Version: v10.15.1
• Platform: Windows 10 Pro 64-bit
• Subsystem: net

If net.Socket loses connection (close is emitted) and the same socket instance is used to reconnect to the same server, no more readable events are emitted (data events are still emitted).

Doesn't work in v10.14/v10.15.1. Works in v8.15.0.

Repro: https://gist.github.com/morkai/fa175bd0104443e6142f3d0e22805653

1. Run server.js
2. Run client.js
3. Client prints client#readable
4. Kill the server
5. Run the server again
6. Client reconnects
7. Client doesn't print any client#readable lines
8. Kill the client
9. Uncomment the data event handler and comment the readable handler in client.js
10. Run the client
11. Client prints client#data
12. Kill the server
13. Run the server again
14. Client reconnects
15. Client resumes printing client#data lines

[morkai]

Stopped working in v10.10.0. Calling socket.read(0) in the connect event handler fixes the issue.

[lpinca]

Reverting 53fb7af1b2 makes 'readable' events to be emitted again upon reconnection.

cc: @mcollina

[lpinca]

@morkai btw, read() should be called until it returns null.

[morkai]

@lpinca even if called without any arguments?

https://nodejs.org/dist/latest-v10.x/docs/api/stream.html#stream_readable_read_size
If the size argument is not specified, all of the data contained in the internal buffer will be returned.

[lpinca]

Looking at the code it seems that that sentence is correct. However a new sentence has been added

https://nodejs.org/dist/latest-v11.x/docs/api/stream.html#stream_readable_read_size
Note that the while loop is necessary when processing data with readable.read(). Only after readable.read() returns null, 'readable' will be emitted.

See #25375 for context.

[mcollina]

This is does not look like a bug in Readable.read. The following behaves correctly:

const { Readable } = require('stream')

const r = new Readable({
  read() {
  }
})

r.push(Buffer.from('hello'))
r.push(Buffer.from(' '))
r.push(Buffer.from('world'))
r.push(null)

r.on('readable', () => {
  console.log('readable')
  let chunk
  while ((chunk = r.read()) !== null) {
    console.log('chunk', chunk.toString())
  }
})

I think this is something specific to how socket.connect works

[addaleax]

@mcollina My guess would be that Readable.prototype._undestroy doesn’t fully reset stream state?

[mcollina]

@mcollina My guess would be that Readable.prototype._undestroy doesn’t fully reset stream state?

That might be the case.

[morkai]

Readable.isPaused() checks whether _readableState.flowing is strictly false.

Before v10.10, _readableState.flowing was always null if in paused mode, so isPaused() returns false (null !== false).

Since v10.10, _readableState.flowing is false if in paused mode, so isPaused() returns true (false === false).

This makes afterConnect() not call read(0): https://github.com/nodejs/node/blob/master/lib/net.js#L1049

See: https://gist.github.com/morkai/a6e8d696f11aa2fb903a369c775e8c76#file-log-txt

[lpinca]

It seems there is also a behavior change between Node.js 8 and Node.js 10, probably wanted but I'm not sure. Consider the following example:

'use strict';

const { Readable } = require('stream');

const buf = Buffer.alloc(8192);

const readable = new Readable({
  read() {
    this.push(buf);
  }
});

readable.on('readable', function() {
  const data = readable.read();
  console.log(data.length);
});

On Node.js 8, it creates (as expected I would say) an endless loop. On Node.js 10 it exits after a few reads. Not sure what happens and if it is expected but at first glance it seems a race condition caused by 'readable' events being emitted on next tick.

[lpinca]

Added confirmed-bug label because I think state.flowing should not be set to false when using only the 'readable' events and readable.read(). As @morkai pointed out, 53fb7af1b2 changed readable.isPaused() behavior under these circumstances.

[ronag]

@lpinca Was this resolved through #26097. Can we close this issue?

[lpinca]

If the original issue is fixed then yes.

[ronag]

@morkai: Is this still an issue?

[morkai]

Yes. The issue is still present in v12.13.1 and v14.1.0.

[ronag]

@morkai: Is it possible for you to instead of re-using the socket, simply create a new one?

@mcollina: I think we briefly discussed at some point that the _undestroy re-use flow is subtly broken. Is this something we should look into fixing? I would much rather deprecate this use-case and ask users to create a new socket instance.

[mcollina]

Fixing would be good. Otherwise deprecating is also an option. It seems this issue is not getting enough attention - or folks willing to work on it.

[morkai]

@morkai: Is it possible for you to instead of re-using the socket, simply create a new one?

I fixed it in the library that was affected by this issue by calling the readable event handler after each connect event to force at least one call to read().

Instead of:

socket.on('connect', () => {
  console.log('connected!');
});

socket.on('readable', () => {
  while (true) {
    const data = socket.read();
    if (!data) break;
    console.log('data', data);
  }
});

this:

socket.on('connect', () => {
  console.log('connected!');
  onReadable();
});

socket.on('readable', onReadable);

function onReadable()
{
  while (true) {
    const data = socket.read();
    if (!data) break;
    console.log('data', data);
  }
}

[mcollina]

I think we can add that to core.