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

'close' event called before 'end' in paused stream #19166

Closed
DiegoTUI opened this issue Mar 6, 2018 · 8 comments
Closed

'close' event called before 'end' in paused stream #19166

DiegoTUI opened this issue Mar 6, 2018 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@DiegoTUI
Copy link
Contributor

DiegoTUI commented Mar 6, 2018

I believe I've found an akward behaviour of a stream in paused mode using the net module. In some specific cases, when the source socket ends, the close event is called before the end event in the destination socket. I've been able to reproduce it simulating the callback queue with nested setImmediates.

Server

'use strict';
                               
const net = require('net');    

let n=0;                       

const opts = {                 
  host: '127.0.0.1',
  port: 1100
};
  
const server = net.createServer(s => {
  let id = n++;                
  s.setEncoding('utf8');       
  s.on('readable', () => {
    setImmediate(() => 
      setImmediate(() => 
        setImmediate(() =>
          setImmediate(() => s.read()))));
  });
  s.on('end', () => console.trace(`Socket ${id} END`));
  s.on('close', () => console.trace(`Socket ${id} CLOSE`));
});

server.listen(opts, console.info.bind(null, 'server listening', opts));

I've tried with two clients.

Client 1 (bash)
$ echo EVENT | nc localhost 1100

Client 2 (node)

'use strict';

const net = require('net');

let n=0;

function runOnce() { 
  const s = net.connect(1100, () => { 
    console.info('Connected', n++);
    s.write('EVENT', () => { 
      s.end();
    });
  });
  return new Promise(ok => { 
    s.on('close', ok)
  })
}

async function run(times) {
  for (let i=0; i<times; i++) { 
    await runOnce();
  }
}

run(10);

It consistently "fails" with both, "fail" meaning that the close event is called before the end event. According to the documentation this should never happen.

The 'close' event is emitted when the stream and any of its underlying resources (a file descriptor, for example) have been closed. The event indicates that no more events will be emitted, and no further computation will occur.

NOTE: The number of setImmediates that you need to nest in order to make it "fail" may vary depending on your environment.

@rohanraj7316
Copy link

rohanraj7316 commented Mar 6, 2018

end event triggers at the ending of the readable side of the socket execute completely.
so, it's not necessary that trigger of close and end events should be sequential.
@DiegoTUI

@DiegoTUI
Copy link
Contributor Author

DiegoTUI commented Mar 6, 2018

@rohanraj7316 but according to the docs, once the close event is emitted, no more events are emitted, right? It seems that in this case, the end event is emitted after the close event contradicting the docs.

@richardlau
Copy link
Member

cc @nodejs/streams

@rohanraj7316
Copy link

can you share the link pls @DiegoTUI

@DiegoTUI
Copy link
Contributor Author

DiegoTUI commented Mar 6, 2018

@rohanraj7316 you mean the link to the docs? Here it is:
https://nodejs.org/dist/latest-v9.x/docs/api/stream.html#stream_event_close_1

@rohanraj7316
Copy link

rohanraj7316 commented Mar 6, 2018

Since we have multiple phases in the Node js event loop.
Close event is not behaving in the way you are
expecting it to be because of the absence of stream callback.

When we say event loop there are multiple phases of Event Loop cycle
all the callback corresponding to given event are present in that phase.
but since you are adding the listener to the net modules object it is acting
according to the net modules close and end callbacks. I guess this can
be one of the possible reason. for more details regarding the behavior of
event you can go through this link
@DiegoTUI

@DiegoTUI
Copy link
Contributor Author

DiegoTUI commented Mar 7, 2018

@rohanraj7316 If I understood correctly, you are suggesting that the fact that the end and close events come in the wrong order could be due to the fact that I am listening to the end and close events of a net.Socket, and not of a "pure" stream. It doesn't make a lot of sense to me because a Socket is a Duplex stream, and it seems that it relies in the stream's event to produce his own events. The docs of the Socket close event are also pretty clear about this:

Emitted once the socket is fully closed. The argument had_error is a boolean which says if the socket was closed due to a transmission error.

If the close event is emitted, then the socket is fully closed. It doesn't make any sense that the end event is emitted after the socket is fully closed.

@mcollina
Copy link
Member

mcollina commented Mar 7, 2018

I think this is a bug. It also goes back to Node 4 at least.

@joyeecheung joyeecheung added stream Issues and PRs related to the stream subsystem. confirmed-bug Issues with confirmed bugs. labels Mar 7, 2018
lpinca added a commit to lpinca/node that referenced this issue Mar 21, 2018
Currently the writable side of the socket is closed as soon as `UV_EOF`
is read regardless of the state of the socket. This allows the handle
to be closed before `'end'` is emitted and thus `'close'` can be
emitted before `'end'` if the socket is paused.

This commit prevents the handle from being closed until `'end'` is
emitted ensuring the correct order of events.

Fixes: nodejs#19166
@lpinca lpinca closed this as completed in 9b7a691 Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants