Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent net/socket behavior #45382

Closed
qdaoming opened this issue Nov 9, 2022 · 7 comments
Closed

Inconsistent net/socket behavior #45382

qdaoming opened this issue Nov 9, 2022 · 7 comments
Labels
net Issues and PRs related to the net subsystem.

Comments

@qdaoming
Copy link
Contributor

qdaoming commented Nov 9, 2022

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
Copy link
Contributor

climba03003 commented Nov 9, 2022

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

@lpinca
Copy link
Member

lpinca commented Nov 9, 2022

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
Copy link
Contributor Author

qdaoming commented Nov 9, 2022

@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
Copy link
Member

lpinca commented Nov 9, 2022

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.

@VoltrexKeyva VoltrexKeyva added the net Issues and PRs related to the net subsystem. label Nov 10, 2022
@qdaoming
Copy link
Contributor Author

@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
Copy link
Member

lpinca commented Nov 11, 2022

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

@bnoordhuis
Copy link
Member

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

@nodejs nodejs locked and limited conversation to collaborators Nov 13, 2022
@bnoordhuis bnoordhuis converted this issue into discussion #45446 Nov 13, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants