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: onFinish will not be triggered again when finished #35845

Closed
wants to merge 5 commits into from

Conversation

rickyes
Copy link
Contributor

@rickyes rickyes commented Oct 28, 2020

When the .end() is called with the payload again after the end, directly return and pass the ERR_STREAM_WRITE_AFTER_END error to the callback and error event.

Fixes: #35833

/cc @ronag @kaizhu256

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 28, 2020
@rickyes rickyes added the wip Issues and PRs that are still a work in progress. label Oct 28, 2020
@rickyes rickyes changed the title http: OnFinish will not be triggered again when finished http: onFinish will not be triggered again when finished Oct 28, 2020
@rickyes rickyes added request-ci Add this label to start a Jenkins CI on a PR. dont-land-on-v10.x and removed wip Issues and PRs that are still a work in progress. labels Oct 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@nodejs-github-bot

This comment has been minimized.

@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@nodejs-github-bot

This comment has been minimized.

@kaizhu256
Copy link
Contributor

kaizhu256 commented Oct 28, 2020

verified latest pr-commit 6e1c697 fixed regression #35833 in windows (using git-bash):

#!/bin/sh
./out/Release/node.exe -e '
require("http").createServer(function (ignore, res) {
    res.on("error", function () {
        return;
    });
    res.end("cc");
    res.end("dd");
}).listen(8081, function () {
    require("http").request("http://localhost:8081").end();
});
setTimeout(process.exit, 5000);
'

# stderr output - none

@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020
@nodejs-github-bot

This comment has been minimized.

lib/_http_outgoing.js Outdated Show resolved Hide resolved
@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2020
@nodejs-github-bot

This comment has been minimized.

@rickyes
Copy link
Contributor Author

rickyes commented Oct 29, 2020

@nodejs/http @ronag @mcollina PTAL, thanks~

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

Can't this be solved just by putting the if (chunk) branch after the if (this.finished) branch?

@rickyes
Copy link
Contributor Author

rickyes commented Oct 31, 2020

Can't this be solved just by putting the if (chunk) branch after the if (this.finished) branch?

@ronag Thanks for you comment, that's what I thought at first, but I realized that this would behave differently than the v14 version, with two problems:

  1. The error code returned when with payload is ERR_STREAM_ALREADY_FINISHED instead of ERR_STREAM_WRITE_AFTER_END
  2. When with payload, only the callback is triggered without sending the error event.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
@nodejs-github-bot

This comment has been minimized.

@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2020
@nodejs-github-bot

This comment has been minimized.

@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2020
@nodejs-github-bot
Copy link
Collaborator

@rickyes rickyes added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 12, 2020

@rickyes rickyes added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 12, 2020
@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 12, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/35845
✔  Done loading data for nodejs/node/pull/35845
----------------------------------- PR info ------------------------------------
Title      http: onFinish will not be triggered again when finished (#35845)
Author     Ricky Zhou <0x19951125@gmail.com> (@rickyes)
Branch     rickyes:fix/http-outgoing-end -> nodejs:master
Labels     author ready, commit-queue, dont-land-on-v10.x, dont-land-on-v12.x, dont-land-on-v14.x, http
Commits    5
 - http: OnFinish will not be triggered again when finished
 - fixup
 - fixup
 - fixup! got error event before error callback
 - fixup! callback order align with stream.Writable
Committers 1
 - rickyes <0x19951125@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/35845
Fixes: https://github.com/nodejs/node/issues/35833
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35845
Fixes: https://github.com/nodejs/node/issues/35833
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-11-12T01:40:04Z: https://ci.nodejs.org/job/node-test-pull-request/34349/
- Querying data for job/node-test-pull-request/34349/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Wed, 28 Oct 2020 07:21:43 GMT
   ✔  Approvals: 3
   ✔  - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/35845#pullrequestreview-526035746
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/35845#pullrequestreview-525944547
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35845#pullrequestreview-529123959
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/360068464

rickyes added a commit that referenced this pull request Nov 12, 2020
PR-URL: #35845
Fixes: #35833
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@rickyes
Copy link
Contributor Author

rickyes commented Nov 12, 2020

Landed in e6e64f7

@rickyes rickyes closed this Nov 12, 2020
@rickyes rickyes deleted the fix/http-outgoing-end branch November 12, 2020 17:27
@rickyes rickyes removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 12, 2020
codebytere pushed a commit that referenced this pull request Nov 22, 2020
PR-URL: #35845
Fixes: #35833
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v15.x regression - ERR_INTERNAL_ASSERTION - failed assert(socket._httpMessage === this);
7 participants