http: onFinish will not be triggered again when finished

[rickyes]

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]

Review requested:

• @nodejs/http
• @nodejs/net

[kaizhu256]

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]

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

[ronag]

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

[rickyes]

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.

[rickyes]

If it is the same as what I said above, this is the correct return order and the conditions for triggering the error event, which are not recorded in the document, I will open a new PR to record these matters and it is also useful for the v14 version.

[ronag]

It’s not supposed to behave like v14. It’s supposed to behave like Writable.

[rickyes]

It’s not supposed to behave like v14. It’s supposed to behave like Writable.

@ronag Not sure which question you are referring to? But I will keep the current view of the error code. and would love to get your thoughts as well, thx @mcollina @addaleax

[ronag]

I’m referring to that you seem to try to make it behave as it did in v14, which is not necessarily correct. You should try to make it behave as a stream.Writable.

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ghost]

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]

Landed in e6e64f7