test: add net regression test

[ronag]

Ensure that the socket is not destroyed when the 'end' event is emitted

Refs: #32780 (comment)

Checklist

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/30661/

[ronag]

This was fixed through a larger semver-major PR on master. This should not land in 14.x unless the referenced PR is included.

[lpinca]

FWIW it is not destroyed before the 'end' event is emitted. It is destroyed after, but the listener where it is destroyed is the first one.

https://github.com/nodejs/node/blob/v13.12.0/lib/net.js#L629-L638

[ronag]

FWIW it is not destroyed before the 'end' event is emitted. It is destroyed after, but the listener where it is destroyed is the first one.

The observed behaviour for a user of the stream is that it is destroyed before 'end' is emitted.

[lpinca]

Not if the user uses emitter.prependListener().

[ronag]

Not if the user uses emitter.prependListener().

I think using prependListener should be discouraged in general (and especially with streams) for this exact reason, it can break ordering assumptions (and also why stream implementations should preferably not depend on public events). Same thing applies with e.g. 'error' where if using prependListener('error') things can break in subtle ways.

I don't think prependListener is commonly used.

[lpinca]

I think using prependListener should be discouraged in general for this exact reason, it can break ordering assumptions ...

While I partially agree, emitter.prependListener() was added and made public for this exact reason (break ordering assumptions).

[ronag]

was added and made public for this exact reason (break ordering assumptions).

Well, if you are using it you should know what you are doing... same with e.g. autoDestroy: false and emitClose: false... with great power...

I do think we should be more explicit about discouraging these things in the docs and make it clear you need to tread carefully.

[lpinca]

I don't mind, I just find this commit message incorrect/misleading. Again the socket is not destroyed before the 'end' event is emitted.

[ronag]

I just find this commit message incorrect/misleading. Again the socket is not destroyed before the 'end' event is emitted.

The test tests that it is not destroyed before 'end' is emitted and it's a test to avoid future regressions. I'm unsure in what way it is misleading?

EDIT: Hm, I guess you are technically right, but it's a bit nit.

[lpinca]

I suggest to use something like "Ensure that the socket is not destroyed when the 'end' event is emitted" to reflect what the test actually does.

[ronag]

Updated

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/30670/ (:yellow_circle:)

[ronag]

Landed in cbe955c