Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: fix close return value mismatch between doc and implementation #51797

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

kylo5aby
Copy link
Contributor

Fixes: #51787

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. needs-ci PRs that need a full CI run. labels Feb 18, 2024
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a test for this.

@nodejs/http I'm a little uncertain on this: since the docs state this should have returned a instance rather than void, is this change semver-major or not (and in that case minor or patch)?

@8JUFtKyf5wQnBAF

This comment has been minimized.

@climba03003
Copy link
Contributor

climba03003 commented Feb 18, 2024

I'm a little uncertain on this: since the docs state this should have returned a instance rather than void

I am not the one you are asking, but from my point of view, it is unintentional change (so, it should be patch) comes from #41263

The behavior of net.Server is not changed until now.

node/lib/net.js

Lines 2236 to 2273 in 17187dd

Server.prototype.close = function(cb) {
if (typeof cb === 'function') {
if (!this._handle) {
this.once('close', function close() {
cb(new ERR_SERVER_NOT_RUNNING());
});
} else {
this.once('close', cb);
}
}
if (this._handle) {
this._handle.close();
this._handle = null;
}
if (this._usingWorkers) {
let left = this._workers.length;
const onWorkerClose = () => {
if (--left !== 0) return;
this._connections = 0;
this._emitCloseIfDrained();
};
// Increment connections to be sure that, even if all sockets will be closed
// during polling of workers, `close` event will be emitted only once.
this._connections++;
// Poll workers
for (let n = 0; n < this._workers.length; n++)
this._workers[n].close(onWorkerClose);
} else {
this._emitCloseIfDrained();
}
return this;
};

@kylo5aby
Copy link
Contributor Author

My perspective is that return instance from net.server.close() is possibly for the sake of facilitating chaining call, based on this, http.server.close() should perhaps maintain the same behavior.

@kylo5aby
Copy link
Contributor Author

@ShogunPanda I have added test cases, and retriggered git Action, but there occurs errors on Windows/macOS as shown below, I think it's doesn't because wrongly run the test cases, can you take a look?
image

@ShogunPanda
Copy link
Contributor

@ShogunPanda I have added test cases, and retriggered git Action, but there occurs errors on Windows/macOS as shown below, I think it's doesn't because wrongly run the test cases, can you take a look? image

@climba03003 Guess who introduced that patch? XD I agree, it's a involuntary change. I would go for semver-patch.

@nodejs/build I don't think it's related to this PR. Can you please investigate?

@kylo5aby
Copy link
Contributor Author

@ShogunPanda I have added test cases, and retriggered git Action, but there occurs errors on Windows/macOS as shown below, I think it's doesn't because wrongly run the test cases, can you take a look? image

@climba03003 Guess who introduced that patch? XD I agree, it's a involuntary change. I would go for semver-patch.

@nodejs/build I don't think it's related to this PR. Can you please investigate?

Yeah, maybe this because of git action error instead of this PR or build problem

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2024
@nodejs-github-bot
Copy link
Collaborator

@ShogunPanda ShogunPanda added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 26, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 26, 2024
@nodejs-github-bot nodejs-github-bot merged commit 60ce078 into nodejs:main Feb 26, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 60ce078

marco-ippolito pushed a commit that referenced this pull request Feb 27, 2024
PR-URL: #51797
Fixes: #51787
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51797
Fixes: #51787
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51797
Fixes: #51787
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#51797
Fixes: nodejs#51787
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http.Server.close return value mismatch between the documentation and the implementation.
10 participants