node net: simplify server.close() behavior when closing unopened server.

I see no use case for behavior where an error is thrown in case a callback is registered for a close event on a closed server. This is bizarre and unusual.

Originally, this behavior was introduced along with commit 984dc057e3 in order to mimic net_legacy behavior. net_legacy for stdio was replaced by uv_net in Node 0.5.7 (2011 AD.).

Up until commit f1dc55d7018, an error would be thrown synchronously if attempt has been made to close an unopen server. The change would introduce the current behavior.

This is a breaking API change and should most likely be introduced in a major update.

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

Jun 24 '20 16:06 mkrawczuk

This PR seems to have gotten a little stuck. Is there anything that should be done to move further? cc: @addaleax

Aug 05 '20 22:08 mkrawczuk

Seems to be stuck again. @nodejs/net, @mcollina ?

Dec 16 '20 15:12 mkrawczuk

@mkrawczuk this needs rebasing.

@nodejs/tsc given this is semver-major, I kindly ask for another review.

Dec 16 '20 15:12 mcollina

@mcollina resolved conflicts via the github interface. I hope this does also rebase.

Dec 16 '20 16:12 mkrawczuk

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

Dec 16 '20 17:12 nodejs-github-bot

Commit Queue failed

- Loading data for nodejs/node/pull/34042
✔  Done loading data for nodejs/node/pull/34042
----------------------------------- PR info ------------------------------------
Title      net: simplify server.close() behavior when closing unopened server. (#34042)
Author     Mateusz Krawczuk  (@mkrawczuk)
Branch     mkrawczuk:net_server_close_simplify -> nodejs:master
Labels     errors, net, semver-major
Commits    5
 - net: simplify server.close() behavior when closing unopened server.
 - Update test/parallel/test-net-server-close.js
 - Update test/parallel/test-net-server-close.js
 - Update test/parallel/test-net-server-close.js
 - Merge branch 'master' into net_server_close_simplify
Committers 2
 - Mateusz Krawczuk 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/34042
Reviewed-By: Matteo Collina 
Reviewed-By: Trivikram Kamat 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34042
Reviewed-By: Matteo Collina 
Reviewed-By: Trivikram Kamat 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-12-16T17:03:04Z: https://ci.nodejs.org/job/node-test-pull-request/34973/
- Querying data for job/node-test-pull-request/34973/
✔  Build data downloaded
   ✖  1 failure(s) on the last Jenkins CI run
   ℹ  This PR was created on Wed, 24 Jun 2020 16:33:28 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/34042#pullrequestreview-486914209
   ✔  - Trivikram Kamat (@trivikr): https://github.com/nodejs/node/pull/34042#pullrequestreview-486490387
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/34042#pullrequestreview-553860517
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/426377355

Dec 16 '20 18:12 github-actions[bot]

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

Dec 16 '20 18:12 nodejs-github-bot

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

Dec 16 '20 21:12 nodejs-github-bot

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

Dec 17 '20 09:12 nodejs-github-bot

Huh? There seems to be a merge conflict in Jenkins job, file lib/net.js. @mcollina do you have an idea for fixing it?

Dec 17 '20 10:12 mkrawczuk

Huh? There seems to be a merge conflict in Jenkins job, file lib/net.js. @mcollina do you have an idea for fixing it?

No

Dec 17 '20 10:12 mcollina

But is it a PR problem or Jenkins problem?

Dec 18 '20 18:12 mkrawczuk

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

Dec 18 '20 20:12 nodejs-github-bot

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

Dec 20 '20 12:12 nodejs-github-bot

This seems to not apply cleanly on master.

13:03:23 Falling back to patching base and 3-way merge...
13:03:23 Auto-merging lib/net.js
13:03:23 CONFLICT (content): Merge conflict in lib/net.js
13:03:23 Auto-merging lib/internal/errors.js
13:03:23 Auto-merging doc/api/net.md
13:03:23 Auto-merging doc/api/errors.md
13:03:23 error: Failed to merge in the changes.
13:03:23 Patch failed at 0001 net: simplify server.close() behavior when closing unopened server.
13:03:23 Use 'git am --show-current-patch' to see the failed patch

Can you please rebase this on top of master?

Dec 20 '20 12:12 mcollina

Hmm, indeed. I have re-rebased it and now it should be all right.

Dec 21 '20 14:12 mkrawczuk

Yep, bad merge/rebase. Now it looks alright. cc @ronag @mcollina

Dec 23 '20 11:12 mkrawczuk