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

mkrawczuk · 2020-06-24T16:33:28Z

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 984dc05 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 f1dc55d, 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.

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

mkrawczuk · 2020-08-05T22:47:04Z

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

mcollina

lgtm

trivikr

LGTM, some minor nits for typos

test/parallel/test-net-server-close.js

mcollina

lgtm

doc/api/errors.md

mkrawczuk · 2020-12-16T15:25:09Z

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

mcollina · 2020-12-16T15:59:19Z

@mkrawczuk this needs rebasing.

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

mkrawczuk · 2020-12-16T16:18:07Z

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

nodejs-github-bot · 2020-12-16T17:03:04Z

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

github-actions · 2020-12-16T18:48:24Z

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

nodejs-github-bot · 2020-12-16T18:54:36Z

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

nodejs-github-bot · 2020-12-16T21:24:49Z

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

nodejs-github-bot · 2020-12-17T09:10:17Z

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

mkrawczuk · 2020-12-17T10:27:15Z

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

mcollina · 2020-12-17T10:28:15Z

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

mkrawczuk · 2020-12-18T18:19:02Z

But is it a PR problem or Jenkins problem?

nodejs-github-bot · 2020-12-18T20:24:57Z

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

nodejs-github-bot · 2020-12-20T12:03:25Z

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

mcollina · 2020-12-20T12:04:30Z

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?

mkrawczuk · 2020-12-21T14:26:24Z

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

ronag

This contains a lot of unrelated commits. Bad rebase/merge?

mkrawczuk · 2020-12-23T11:30:58Z

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

ronag · 2020-12-23T11:33:47Z

lib/net.js

+
+    if (typeof cb === 'function') {
+      this.once('close', cb);
+    }


@mcollina: This change means that the callback won't always be invoked. Isn't that considered an anti pattern?

As long as callback is a function, it will be called. Or am I missing something?

@ronag is right. If there is no _handle yet the callback will not be called.

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 984dc05 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 f1dc55d, 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.

mcollina

Now that I see this, I agree with @ronag. A user would expect that a callback is called.

This change is likely not correct as it is right now.

mcollina · 2021-01-16T18:43:17Z

test/parallel/test-net-server-close.js

+
+// Ensure that the close callback is not called when the server was not
+// previously running.
+server.close(common.mustNotCall());


This is not correct. We should always call the callback.

@mcollina could you please clarify why we need to close the server if that server is not running?

FWIW, I agree with Matteo. The callback here should be called in case there is any cleanup logic that is deferred to close (e.g. closing fd's). There's a non-trivial risk of breakage here and I don't see this change as being worth it now that I reconsider.

reconsidered position

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. net Issues and PRs related to the net subsystem. labels Jun 24, 2020

mkrawczuk requested a review from a team as a code owner August 10, 2020 16:06

mkrawczuk requested a review from a team August 10, 2020 16:06

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 10, 2020

mcollina approved these changes Aug 10, 2020

View reviewed changes

trivikr approved these changes Sep 11, 2020

View reviewed changes

test/parallel/test-net-server-close.js Outdated Show resolved Hide resolved

test/parallel/test-net-server-close.js Outdated Show resolved Hide resolved

test/parallel/test-net-server-close.js Outdated Show resolved Hide resolved

mcollina approved these changes Sep 11, 2020

View reviewed changes

ronag reviewed Sep 11, 2020

View reviewed changes

doc/api/errors.md Show resolved Hide resolved

mcollina requested review from jasnell and removed request for a team December 16, 2020 15:59

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2020

jasnell previously approved these changes Dec 16, 2020

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2020

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2020

github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 16, 2020

mcollina added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 16, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2020

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2020

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2020

ronag removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2020

ronag requested changes Dec 21, 2020

View reviewed changes

mkrawczuk force-pushed the net_server_close_simplify branch from 59a3a94 to b0e2b2e Compare December 23, 2020 11:29

ronag reviewed Dec 23, 2020

View reviewed changes

mkrawczuk force-pushed the net_server_close_simplify branch from b0e2b2e to b86ca35 Compare January 16, 2021 13:34

mcollina requested changes Jan 16, 2021

View reviewed changes

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mkrawczuk commented Jun 24, 2020

mkrawczuk commented Aug 5, 2020

mcollina left a comment

trivikr left a comment

mcollina left a comment

mkrawczuk commented Dec 16, 2020

mcollina commented Dec 16, 2020

mkrawczuk commented Dec 16, 2020

nodejs-github-bot commented Dec 16, 2020

github-actions bot commented Dec 16, 2020

nodejs-github-bot commented Dec 16, 2020

nodejs-github-bot commented Dec 16, 2020

nodejs-github-bot commented Dec 17, 2020

mkrawczuk commented Dec 17, 2020

mcollina commented Dec 17, 2020

mkrawczuk commented Dec 18, 2020

nodejs-github-bot commented Dec 18, 2020

nodejs-github-bot commented Dec 20, 2020

mcollina commented Dec 20, 2020

mkrawczuk commented Dec 21, 2020

ronag left a comment

mkrawczuk commented Dec 23, 2020

ronag Dec 23, 2020

mkrawczuk Jan 16, 2021

mcollina Jan 16, 2021

mcollina left a comment

mcollina Jan 16, 2021

Mifrill Nov 10, 2023

jasnell Nov 11, 2023

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

Are you sure you want to change the base?

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

Conversation

mkrawczuk commented Jun 24, 2020

mkrawczuk commented Aug 5, 2020

mcollina left a comment

Choose a reason for hiding this comment

trivikr left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mkrawczuk commented Dec 16, 2020

mcollina commented Dec 16, 2020

mkrawczuk commented Dec 16, 2020

nodejs-github-bot commented Dec 16, 2020

github-actions bot commented Dec 16, 2020

nodejs-github-bot commented Dec 16, 2020

nodejs-github-bot commented Dec 16, 2020

nodejs-github-bot commented Dec 17, 2020

mkrawczuk commented Dec 17, 2020

mcollina commented Dec 17, 2020

mkrawczuk commented Dec 18, 2020

nodejs-github-bot commented Dec 18, 2020

nodejs-github-bot commented Dec 20, 2020

mcollina commented Dec 20, 2020

mkrawczuk commented Dec 21, 2020

ronag left a comment

Choose a reason for hiding this comment

mkrawczuk commented Dec 23, 2020

ronag Dec 23, 2020

Choose a reason for hiding this comment

mkrawczuk Jan 16, 2021

Choose a reason for hiding this comment

mcollina Jan 16, 2021

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina Jan 16, 2021

Choose a reason for hiding this comment

Mifrill Nov 10, 2023

Choose a reason for hiding this comment

jasnell Nov 11, 2023

Choose a reason for hiding this comment