Inconsistent net/socket behavior

[qdaoming]

Version

v20.0.0-pre

Platform

Linux xeon-intel-6252N 5.15.0-41-generic #44~20.04.1-Ubuntu SMP Fri Jun 24 13:27:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

net

What steps will reproduce the bug?

1. launch the server code below: >node myserver.js
2. launch client code below in another console: >node myclient.js
3. client will stuck there after printing ('afterWrite').
4. modify buf's length in myclient.js to 2500*1024, launch client again. All works well.

---

client side code (myclient.js):

var net = require('net');
//2500KB is OK,2600KB will deadlock
var buf = new Uint8Array(2600*1024);
var client = new net.Socket();
client.connect(1337, '127.0.0.1', function() {
        client.write(buf, function() {
          console.log('all content write finished');
          client.destroy();
        });
        console.log('afterWrite');
});

server side code (myserver.js):

var net = require('net');
var server = net.createServer(function(socket) {
        console.log('server connected');
        //uncomment following line can solve the client dead issue
        // socket.on('data', function (data) { });
});
server.listen(1337, '127.0.0.1');

How often does it reproduce? Is there a required condition?

It always happen with my tested node binary from nodev16 to nodev20.

What is the expected behavior?

The client will exit and print the console output as (when client's buf length is 2500*1024):
afterWrite
all content write finished

What do you see instead?

The client will stuck(dead) and print the console output as (when client's buf length is 2600*1024):
afterWrite

Additional information

Such inconsistent behavior is quite misleading. Node.js should represent consistent behavior whatever the buf length is.

There are several workarounds:

1. modify the server code to add socket.ondata callback
2. modify node.js lib/internal/streams/readable.js addChunk() function to check ondata listener count

[climba03003]

I am using Windows WSL and do not face the same issue.

[lpinca]

modify the server code to add socket.ondata callback

Yes, the reason for this is that data is not flowing. The buffer is filled and the socket.write() callback is not called. This is expected.

[qdaoming]

@lpinca the current node.js impl is quite confusing. Why increasing the sent data by just 100KB will make the client dead?
It is not good for developers.

[lpinca]

Why increasing the sent data by just 100KB will make the client dead?

Because it fills the buffer and the other peer is not reading. Also, the client is not dead. If you keep writing, socket.write() will return false informing the user/application that no more data should be written.

[qdaoming]

@lpinca Such inconsistent behavior is quite confusing and unfriendly to developers.
And socket.on('data',listener) and socket.on('readable',listener) are optional listener.
My suggested solution for this issue is that:
If both stream's 'data' listener and 'readable' listener are missing, node.js will discard the data read from the bottom layer.
Do you think this solution is OK?

[lpinca]

No, it shouldn't discard anything. The stream is "paused". This is how all streams work in Node.js.

[qdaoming]

@lpinca @bnoordhuis
There is a similar processing in http_server implementation
(i.e. dump /discard the http incoming data)
https://github.com/nodejs/node/blob/3ebe7535b456771078368ba72345f8d2e16ce695/lib/_http_server.js#L924
This processing is consistent with my suggested fix above.
I thought that we should add such fix in node.js socket code.

[lpinca]

This is for HTTP and it is done on purpose on the specific case where the user called response.end(). Discarding in this case makes sense. Discarding unconditionally does not make sense and is wrong.

[bnoordhuis]

This seems to be a misunderstanding more than a bug so I'll go ahead and convert it to a discussion.