http: don't write error to socket #34465

ronag · 2020-07-21T18:54:57Z

The state of the connection is unknown at this point and
writing to it can corrupt client state before it is aware
of an error.

Checklist

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

ronag · 2020-07-21T18:55:30Z

Maybe a bit too strict but figuring out whether it's actually safe to write to the socket or not is non trivial.

The state of the connection is unknown at this point and writing to it can corrupt client state before it is aware of an error.

lpinca

It's best effort, I don't think we should change this.

nodejs-github-bot · 2020-07-21T19:20:30Z

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

ronag · 2020-07-21T19:23:44Z

It's best effort, I don't think we should change this.

Why? It can corrupt responses without the client being able to detect it?

lpinca · 2020-07-21T19:28:42Z

What is the advantage in removing the response? It's a single chunk which is written in a single write with no other pending writes.

ronag · 2020-07-21T19:30:11Z

It's a single chunk which is written in a single write with no other pending writes.

I haven't followed the code fully but I believe e.g. bad response could be written after the http headers are written and be parsed as part of the http response body.

In the case of 'upgrade' (websocket) and 'connect' I'm not sure what actually happens.

ronag · 2020-07-21T19:32:42Z

@nodejs/http

lpinca · 2020-07-21T19:34:41Z

On upgrade the parser is freed and the socketOnError listener removed. And if the client writes an invalid upgrade request a 400 response if expected.

ronag · 2020-07-21T19:38:12Z

On upgrade the parser is freed and the socketOnError listener removed. And if the client writes an invalid upgrade request a 400 response if expected.

Cool. That leaves the after headers are written scenario.

dougwilson · 2020-07-21T19:47:55Z

I don't have specific opinion to add to this PR, but just wanted to link the original PR #15324 that added the first response writing if any of the conversation from that thread helps here.

lpinca · 2020-07-21T19:51:00Z

Cool. That leaves the after headers are written scenario.

I do not remember any bug report opened for this scenario. Do you mean something like this?

Part of the response is written when the request is in progress.
The request triggers a 'clientError'.

ronag · 2020-07-21T19:51:29Z

Ok. Changed to only write if nothing else has been written to the socket.

ronag · 2020-07-21T19:53:55Z

Cool. That leaves the after headers are written scenario.

I do not remember any bug report opened for this scenario.

There is non as far as I know.

Do you mean something like this?

Part of the response is written when the request is in progress.

The request triggers a 'clientError'.

The socket emits 'error' at any point after the headers has been written and the body has not yet completed writing. Writes can still succeed after 'error' is emitted.

lpinca · 2020-07-21T19:55:29Z

Looks better now.

nodejs-github-bot · 2020-07-21T19:59:19Z

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

nodejs-github-bot · 2020-07-21T20:40:44Z

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

mcollina

lgtm

wa-Nadoo · 2020-07-21T21:52:14Z

I think the 'clientError' event documentation should be updated too https://nodejs.org/api/http.html#http_event_clienterror

ronag · 2020-07-21T21:56:49Z

@wa-Nadoo PTAL

ronag · 2020-07-25T08:00:01Z

Landed in e8d7fed

The state of the connection is unknown at this point and writing to it can corrupt client state before it is aware of an error. PR-URL: #34465 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2020-07-27T22:38:27Z

This change is breaking a test on v14.x

=== release test-http-1.0 ===
Path: parallel/test-http-1.0
assert.js:103
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ ''
- 'HTTP/1.1 200 OK\r\n' +
-   'Content-Type: text/plain\r\n' +
-   'Connection: close\r\n' +
-   'Transfer-Encoding: chunked\r\n' +
-   '\r\n' +
-   '7\r\n' +
-   'Hello, \r\n' +
-   '6\r\n' +
-   'world!\r\n' +
-   '0\r\n' +
-   '\r\n'
    at response_validator (/Users/mylesborins/code/node/v14.x/test/parallel/test-http-1.0.js:158:12)
    at Socket.<anonymous> (/Users/mylesborins/code/node/v14.x/test/parallel/test-http-1.0.js:54:7)
    at Socket.<anonymous> (/Users/mylesborins/code/node/v14.x/test/common/index.js:363:15)
    at Socket.emit (events.js:326:22)
    at endReadableNT (_stream_readable.js:1244:12)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '',
  expected: 'HTTP/1.1 200 OK\r\n' +
    'Content-Type: text/plain\r\n' +
    'Connection: close\r\n' +
    'Transfer-Encoding: chunked\r\n' +
    '\r\n' +
    '7\r\n' +
    'Hello, \r\n' +
    '6\r\n' +
    'world!\r\n' +
    '0\r\n' +
    '\r\n',
  operator: 'strictEqual'
}

Any idea what's going on? Should this be backported?

The state of the connection is unknown at this point and writing to it can corrupt client state before it is aware of an error. PR-URL: #34465 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ronag added the http Issues or PRs related to the http subsystem. label Jul 21, 2020

ronag requested review from jasnell and addaleax July 21, 2020 18:54

http: don't write error to socket

6657553

The state of the connection is unknown at this point and writing to it can corrupt client state before it is aware of an error.

ronag force-pushed the http-error-write branch from 81b3062 to 6657553 Compare July 21, 2020 18:56

lpinca requested changes Jul 21, 2020

View reviewed changes

fixup: linting

40f7def

fixup: altenrative

56a12f3

lpinca approved these changes Jul 21, 2020

View reviewed changes

fixup: add test

9b70294

mcollina approved these changes Jul 21, 2020

View reviewed changes

fixup: update docs

08a3fa9

jasnell approved these changes Jul 21, 2020

View reviewed changes

ronag closed this Jul 25, 2020

MylesBorins added the backport-requested-v14.x label Jul 27, 2020

ruyadorno mentioned this pull request Jul 28, 2020

v14.7.0 proposal #34542

Merged

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

mhdawson mentioned this pull request Jan 18, 2022

pummel/test-worker-take-heapsnapshot - crash in debug mode on arm #41204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: don't write error to socket #34465

http: don't write error to socket #34465

ronag commented Jul 21, 2020 •

edited

Loading

ronag commented Jul 21, 2020

lpinca left a comment

nodejs-github-bot commented Jul 21, 2020

ronag commented Jul 21, 2020

lpinca commented Jul 21, 2020

ronag commented Jul 21, 2020 •

edited

Loading

ronag commented Jul 21, 2020

lpinca commented Jul 21, 2020 •

edited

Loading

ronag commented Jul 21, 2020

dougwilson commented Jul 21, 2020

lpinca commented Jul 21, 2020

ronag commented Jul 21, 2020 •

edited

Loading

ronag commented Jul 21, 2020

lpinca commented Jul 21, 2020

nodejs-github-bot commented Jul 21, 2020

nodejs-github-bot commented Jul 21, 2020

mcollina left a comment

wa-Nadoo commented Jul 21, 2020

ronag commented Jul 21, 2020

ronag commented Jul 25, 2020

MylesBorins commented Jul 27, 2020

http: don't write error to socket #34465

http: don't write error to socket #34465

Conversation

ronag commented Jul 21, 2020 • edited Loading

Checklist

ronag commented Jul 21, 2020

lpinca left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 21, 2020

ronag commented Jul 21, 2020

lpinca commented Jul 21, 2020

ronag commented Jul 21, 2020 • edited Loading

ronag commented Jul 21, 2020

lpinca commented Jul 21, 2020 • edited Loading

ronag commented Jul 21, 2020

dougwilson commented Jul 21, 2020

lpinca commented Jul 21, 2020

ronag commented Jul 21, 2020 • edited Loading

ronag commented Jul 21, 2020

lpinca commented Jul 21, 2020

nodejs-github-bot commented Jul 21, 2020

nodejs-github-bot commented Jul 21, 2020

mcollina left a comment

Choose a reason for hiding this comment

wa-Nadoo commented Jul 21, 2020

ronag commented Jul 21, 2020

ronag commented Jul 25, 2020

MylesBorins commented Jul 27, 2020

ronag commented Jul 21, 2020 •

edited

Loading

ronag commented Jul 21, 2020 •

edited

Loading

lpinca commented Jul 21, 2020 •

edited

Loading

ronag commented Jul 21, 2020 •

edited

Loading