streams: calling end while ending will never invoke callback?

[ronag]

Calling end() twice with callback will cause the second callback to never be invoked:

e.g. the following will fail.

writable.end(common.mustCall());
writable.end(common.mustCall());

I'm not sure what the behavior should be here. Maybe calling the callback with an error? Either way, not calling the callback at all seems to me like it will cause problems and memory leaks.

[ronag]

@mcollina ping

[ronag]

http/1 has the same "problem"

[lpinca]

The callback is added as a listener of the 'finish' event so the behavior seems correct to me. The event is emitted only once.

[lpinca]

There is no memory leak because the callback is used only on the first .end() call.

[mcollina]

I think adding an error to the callback if the stream has already emitted end could be accepted. We need to verify this would not break CITGM.

There should be enough state on Readable around to easily add a check.

[lpinca]

I think adding an error to the callback if the stream has already emitted end could be accepted

You mean 'finish'? And what would you do with all callbacks between the first writable.end() and the actual 'finish' event? For example.

writable.on('finish', () => {
  writable.end(() => {
    // Called with an error.
  });
});

writable.end(() => {
  // Called when `'finish'` is emitted.
});

writable.end(() => {
  // Called when `'finish'` is emitted?
});

writable.end(() => {
  // Called when `'finish'` is emitted?
});

// ...

[ronag]

https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L592

if ending the callback is never registered anywhere?

[lpinca]

Yes.

[ronag]

maybe?

writable.on('finish', () => {
  writable.end((err) => {
  	// error
  });
});

writable.end((err) => {
 // ok
});

writable.end((err) => {
  // error
});

writable.end((err) => {
  // error
});

I'm unsure...

[addaleax]

@ronag I think that suggestion is just fine 👍 The only real alternative I could see is also calling the callbacks for the subsequent .end() calls, but one really should only have one .end() call…

[ronag]

This has been sorted