socket: don't emit premature 'close'

[ronag]

I'm not sure how to get this to work properly nor how to create a test for it but I think there is an issue here to fix.

Looking at the current net.Socket implementation I see a few potential problems:

• 'close' can be emitted twice. Once from https://github.com/nodejs/node/blob/master/lib/net.js#L597 and once from https://github.com/nodejs/node/blob/master/lib/net.js#L604. Once with hadError argument and once without.

• EDIT: Doesn't emit close if !_handle.

• EDIT: _server._emitCloseIfDrained before socket is closed.

• 'close' can be prematurely emitted before the handle is actually closed.

• 'close' has an argument which doesn't conform with the streams spec.

• if the handle calls the callback synchronously then 'close' is emitted while destroyed !== false.

• Some user land code expect 'close' to have no arguments, e.g.

function onComplete (err) {
  if (err) {
    ...
  } else {
    ...
  }
}

socket.on('error', onComplete)
socket.on('close', onComplete)

Also, I believe the spec for stream events says that 'close' has no arguments?

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[lpinca]

Good catch, it looks like a regression introduced with a7c25b7

Also, I believe the spec for stream events says that 'close' has no arguments?

hadError is emitted along with the event for net.Socket

cc: @mafintosh

[BridgeAR]

@nodejs/streams @nodejs/http2 @mcollina @apapirovski PTAL

[ronag]

hadError is emitted along with the event for net.Socket

This is a bit difficult to implement currently though, since close is emitted by the base class and there is currently no good way to do this without overriding Stream.destroy or (as before) emitting close twice.

[ronag]

Related, #20755

[alexjeffburke]

Sorry to have to ask but this feels like something that could cause quite subtle changes in behaviour - I can’t figure out whether this shipped in node 10, but another related change was previously reverted. If it has shipped, should the original commit be reverted for the 10.2 release?

[lpinca]

Correct, I missed this commit 5e3f516.

Can you please update the PR description?

• 'close' can be prematurely emitted before the handle is actually closed.

No, it is emitted when the handle is closed.

if the handle calls the callback synchronously then 'close' is emitted while destroyed !== false.

The callback of _handle.close() is not called synchronously.

To summarise, only the following points are valid:

• Doesn't emit close if !_handle
• 'close' has an argument which doesn't conform with the streams spec.

I think both of them are expected and work as intended.

[ronag]

@lpinca what I'm still unsure about is since error will be emitted at the next tick which is scheduled AFTER handle.close whether error might be emitted after close.

[lpinca]

That doesn't seem to happen, it is hopefully covered by tests and if not it would have already been reported.

[lpinca]

If I'm not wrong this will emit an error every time socket.destroy() is called with an error.

[ronag]

That will happen through cb(exception) as well?

[ronag]

yes, you are right, this is quite complicated...

[alexjeffburke]

@ronag @lpinca thanks both very much for continuing to dig into this. The reason I somewhat pushed back a bit was I remember many times we've have to on('error', ..) and on('close', ..) in codebases and use a 'sawEnd` boolean to make sure we detect teardown situations but avoid cleanup code being called twice due to these events etc. Hopefully that helps explains my earlier nerves a little more :)

[lpinca]

I'm not going to block this but I'm not very happy with removing the hadError argument. net and fs streams are a bit special and userland modules have been modelled around their behavior over the years. Removing an argument that has been there since version v0.1.90 for the sake of consistency doesn't seem a good idea to me.

[lpinca]

I didn't test but I think this still does not prevent the 'error' event from being emitted multiple times. Assume that socket.destroy(err) is called twice.

First call will wait for this callback to be called and emit 'error' and 'close' when that happens.
Second call will find _readableState.destroyed set to true but _writableState.errorEmitted is not set to true yet as that will happen when this callback if invoked, so another 'error' event will be emitted.

[ronag]

But isn't that true for any stream? If you call destroy twice with error it will emit error twice... I would say the problem is in the destroy impl then?

[lpinca]

No, it's emitted only once

const { Writable } = require('stream');

const w = new Writable();

w.on('error', console.log);
w.destroy(new Error('Oops'));
w.destroy(new Error('Oops'));

[ronag]

Twice like this...

const { Writable } = require('stream');

const w = new Writable({
    destroy (err, cb) {
        process.nextTick(() => cb(err))
    }
});

w.on('error', console.log);
w.destroy(new Error('Oops'));
w.destroy(new Error('Oops'));

it's only once if you call cb synchronously. Which doesn't seem like the usual case or we wouldn't have a callback?

[lpinca]

The callback on destroy is not a public api. It is used only internally.

[ronag]

I'm confused... https://nodejs.org/dist/latest-v10.x/docs/api/stream.html#stream_constructor_new_stream_writable_options and https://nodejs.org/dist/latest-v10.x/docs/api/stream.html#stream_writable_destroy_err_callback

[lpinca]

You are right, sorry, I was referring to https://nodejs.org/api/stream.html#stream_writable_destroy_error.

[mafintosh]

(We should fix that error being emitted more than once in streams imo.)

[ronag]

I'm not going to block this but I'm not very happy with removing the hadError argument. net and fs streams are a bit special and userland modules have been modelled around their behavior over the years.

Alternatively, I would suggest then that the hadError option be added to the streams API? So that future userland modules modeled around the streams API doesn't incorrectly assume no argument.

I'm mostly concerned about the unexpected difference in behavior. I've had bugs in my own code due to this.

[mcollina]

I'm -1 to land this change right now. I prefer we land this after some of the refactoring in streams described in #20096.

Anyway, a unit test would be really handy.

[jasnell]

Given the -1's and the lack of progress... closing this one for now.