Skip to content
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

net: fix readable event not emitted (fixes #25969) #48866

Closed
wants to merge 3 commits into from

Conversation

CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Jul 21, 2023

fix readable event not emitted after reconnect

Closes: #25969

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Jul 21, 2023
fix readable event not emitted  after reconnect

Fixes: nodejs#25969
@CGQAQ CGQAQ force-pushed the net-fix-25969 branch 2 times, most recently from 1bb06be to 07b336d Compare July 21, 2023 09:50
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening a PR! Can you please add a unit test?

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jul 21, 2023

Thanks for opening a PR! Can you please add a unit test?

I'll add it later

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jul 22, 2023

@mcollina I'm sorry, but I don't know how to test this, I have tested it locally, it works now.
I have two files, and I need to do the following

  1. node server.js
  2. node client.js
  3. ctrl c node server.js
  4. see client.js output
  5. run node server.js again
  6. see client.js output keep going

please help me.

client.js

const net = require('net');

const client = new net.Socket();
let connected = false;

client.on('error', (err) => console.error('client#error', err.message));

client.on('close', () =>
{
  console.log('client#close');
  
  connected = false;
  
  setTimeout(connect, 333);
});

client.on('connect', () =>
{
  console.log('client#connect');
  
  connected = true;
});

//client.on('data', (data) => console.log('client#data', data));
///*
client.on('readable', () =>
{
  const data = client.read();
  
  if (data)
  {
    console.log('client#readable', data);
  }
});
//*/

connect();

setInterval(() => 
{
  if (connected)
  {
    client.write(Buffer.from([0, 1, 2]));
  }
}, 333);

function connect()
{
  console.log('connecting...');
  
  client.connect(502, '127.0.0.1');
}

server.js

const net = require('net');

const server = net.createServer();

server.on('connection', (conn) =>
{
  console.log('server#connection');
  
  conn.on('error', (err) => console.error('conn#error', err.message));
  conn.on('close', (err) => console.log('conn#close'));
  conn.on('readable', () =>
  {
    const data = conn.read();
    
    if (data)
    {
      console.log('conn#readable', data);
      
      conn.write(data);
    }
  });
});

server.on('error', (err) => console.error(err.message));

server.on('close', () => console.log('server#close'));

server.on('listening', () => console.log('server#listening'));

server.listen(502);

@mcollina
Copy link
Member

It might be passing, but it is now failing this test: https://github.com/nodejs/node/blob/main/test/parallel/test-net-connect-paused-connection.js.


As to write a test, you should combine your server and client into one file and place it inside test/parallel. Take a look at other tests too! Uou'll see that you need to add a require to ../common for example.

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jul 22, 2023

I tried this, but client.on('end' wont trigger after server close, I didn't see any test failed @mcollina

image
test I tried

// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

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

const net = require('net');

const N = 50;
let client_recv_count = 0;
let client_end_count = 0;
let client_readable_count = 0;
let disconnect_count = 0;

const server = net.createServer(function(socket) {
  console.error('SERVER: got socket connection');
  
  console.error('SERVER connect, writing');
  socket.write('hello\r\n');

  socket.on('end', () => {
    console.error('SERVER socket end, calling end()');

    socket.end();
  });

  socket.on('close', (had_error) => {
    console.log(`SERVER had_error: ${JSON.stringify(had_error)}`);
    assert.strictEqual(had_error, false);
  });
});

server.listen(0, function() {
  console.log('SERVER listening');
  const client = net.createConnection(this.address().port);

  client.setEncoding('UTF8');

  client.on('connect', () => {
    console.error('CLIENT connected');
  });

  client.on("readable", () => {
    console.log('CLIENT readable');
    client_readable_count++;

    client_recv_count += 1;
    console.log(`client_recv_count ${client_recv_count}`);
    console.error('CLIENT: calling end');
    client.end(); 
  });

  client.on('end', () => {
    console.error('CLIENT end');
    client_end_count++;
  });

  client.on('close', (had_error) => {
    console.log('CLIENT disconnect');
    assert.strictEqual(had_error, false);
    if (disconnect_count++ < N)
      client.connect(server.address().port); // reconnect
    else
      server.close();
  });
});

process.on('exit', () => {
  assert.strictEqual(disconnect_count, N + 1);
  assert.strictEqual(client_recv_count, N + 1);
  assert.strictEqual(client_end_count, N + 1);
  assert.strictEqual(client_readable_count, N + 1);
});

Result(hangs on last line because client side close didn't trigger):
image

@@ -1033,6 +1033,8 @@ function internalConnect(
self, address, port, addressType, localAddress, localPort, flags) {
// TODO return promise from Socket.prototype.connect which
// wraps _connectReq.
debug('resume');
self.resume();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only resume if flowing? Or we might lose data when reconnecting a paused stream. @mcollina

Copy link
Contributor Author

@CGQAQ CGQAQ Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But flowing is always false after connected
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that if the socket is paused or never started, you will force it to start. Causing lost data for the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ronag.
Could you just check if any data event is there (which implies flowing)?

Also please consider to add whatever solution you find here in internalConnectMultiple, otherwise it will fail with network family autoselection.

@ronag
Copy link
Member

ronag commented Jul 24, 2023

FWIW. I think the whole undestroy/reconnect flow for net.Socket is fundamentally flawed and I would personally prefer to just doc deprecate it and emit a warning to anyone that uses it. We should recommend creating a new socket instance instead.

@mcollina
Copy link
Member

mcollina commented Jul 24, 2023

FWIW. I think the whole undestroy/reconnect flow for net.Socket is fundamentally flawed and I would personally prefer to just doc deprecate it and emit a warning to anyone that uses it. We should recommend creating a new socket instance instead.

I agree

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jul 25, 2023

I think you guys are right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

readable event not emitted after net.Socket reconnects
5 participants