http: don't error after http client response #28668

ronag · 2019-07-13T12:35:06Z

This tries to solve #27916. Where an error can be emitted from a HttpClient request after the response has been ended.

In particular see the comment #27916 (comment)

Depends on #28621.

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 · 2019-07-13T12:35:18Z

@benjamingr ping

ronag · 2019-07-13T12:44:44Z

The main question here is:

"What should happen if writing to a request who's response has ended?"

Should it return true/false? Should it emit an error?

Even though emitting some form of error might seem the most semantically correct it makes the API very hard to use. I would prefer a noop + a dump (true) or no more data (false).

lpinca · 2019-07-13T12:56:55Z

Even though emitting some form of error might seem the most semantically correct it makes the API very hard to use. I would prefer a noop + a dump (true) or no more data (false).

Users have no way to know they are doing something wrong if it fails silently.

ronag · 2019-07-13T13:01:16Z

Users have no way to know they are doing something wrong if it fails silently.

I would argue that they are not doing anything wrong... it's perfectly valid to end a response before receiving the whole request.

ronag · 2019-07-13T13:06:07Z

Hm, maybe you are right. I'm having trouble wrapping my head around this one.

lpinca · 2019-07-13T13:06:14Z

In my opinion intentional or unintentional writing to ClientRequest after the 'response' event is emitted is a user error.

ronag · 2019-07-13T13:08:09Z

@lpinca how about this version?

ronag · 2019-07-13T13:08:37Z

Though ERR_WRITE_AFTER_END is not strictly true either. Do we need a new error for this?

lib/_http_outgoing.js

ronag · 2019-07-13T13:49:21Z

@lpinca: Question. Is it valid to write data to a request after having received a response and the response has not ended? I'm a bit unsure. If no, then this issue is easier to resolve.

i.e. is something like this valid:

server.on('request', (req, res) => req.pipe(res))

ronag · 2019-07-13T13:52:19Z

@lpinca another try...

lpinca · 2019-07-13T13:54:45Z

Is it valid to write data to a request after having received a response and the response has not ended? I'm a bit unsure.

I think it is invalid, it does not make much sense.

ronag · 2019-07-13T13:59:22Z

Ok. That works for me. Should we have a new error? e.g. ‘WRITE_AFTER_RESPONSE’?

lpinca · 2019-07-13T14:00:55Z

All data is actually discarded

node/lib/_http_server.js

Line 629 in 84ad92d

req._dump();

if the user is not consuming the data.

lpinca · 2019-07-13T14:01:37Z

Ok. That works for me. Should we have a new error? e.g. ‘WRITE_AFTER_RESPONSE’?

Works for me.

lpinca · 2019-07-13T14:13:46Z

On the server, does it make sense to keep reading the request (if not complete) if you are already writing the response? I'm not sure, I think not.

ronag · 2019-07-13T14:48:55Z

What does the spec say?

ronag · 2019-07-13T14:50:21Z

All data is actually discarded

node/lib/_http_server.js

Line 629 in 84ad92d

req._dump();

if the user is not consuming the data.

This is after response has finished.

lpinca · 2019-07-13T14:54:57Z

The issue happens after response is finished (server-side) when socket is destroyed

ronag · 2019-07-13T15:11:31Z

I’m confused. Is the PR fine as is? I.e. we error if the response is finished. Or when the response has started?

lpinca · 2019-07-13T15:44:19Z

I think it will not work if we wait for it to be finished because the write can occur before res.closed is set to true, and the server might have already destroyed the socket. Hope it makes sense.

ronag · 2019-07-13T16:51:32Z

@lpinca updated according to our discussion

lpinca · 2019-07-13T17:50:50Z

Ok this is more or less what I had in mind with #27916 (comment). Btw we still have an error so I'm not sure if it is much better than original behavior.

lpinca · 2019-07-13T17:54:01Z

lib/_http_client.js


+ClientRequest.prototype.write = function write(chunk, encoding, callback) {
+  if (this.res) {
+    this.emit('error', new ERR_REQUEST_WRITE_AFTER_RESPONSE());


This will allow the 'error' event to be emitted multiple times, are there other case where this can happen?

The alternative would be worse though (throwing outside) no?

If I'm not wrong the original ECONNRESET is forwarded to the ClientRequest instance.

benjamingr · 2019-07-13T18:09:56Z

Hmm, can we get implementor feedback from express/fastify?

I am also more in favor of "this should error" but I am really bit sure.

ronag · 2019-07-13T20:49:25Z

I'm more in favor of not error. Since that would be less "painful". This error is not harmful per se and will cause things that don't necessarily have to fail, to fail.

But I'm fine with either as long as it is consistent and predictable. But I do require it requires some additional fixes such as #28672.

It's a bit similar to the considerations in #20077.

lpinca · 2019-07-14T05:20:50Z

@dougwilson

dougwilson · 2019-07-14T05:40:48Z

Hmm, can we get implementor feedback from express/fastify?

The changes as they are currently in the PR look like they only deal with the Node.js HTTP client (vs the Node.js HTTP server which would be express/fastify/etc.), is that right?

I tried to read though and understand the conversation above as well, though. If looking for general HTTP/1 guidance, though, the HTTP sever is allowed to start writing the response before the client has completed writing the HTTP request to the wire; notably the 1xx series of response codes, where the client expects multiple HTTP responses to a single request.

This can sometimes be taken advantage of by servers to provide upload progress to the clients; I have worked on software that took advantage of this by immediately sending back a HTTP 200 HTML response before a multipart request was completed with incomplete HTML, trickling down more HTML to update a progress bar for the user to see. The down side is that intermediaries between the end-user and the server can make it not work as intended.

This specific technique has become obsolete with XHR 2's progress event tracking in JavaScript if using a FormData object to upload, these days, but just used it as a read-world example of a server sending back response data while the request data is still being sent, if there was a question on that in the conversation. RFC 723x series does not provide specific guidance around this that I know of, so it's open for servers and clients to interpret.

I hope that information helps with this PR.

lpinca · 2019-07-14T05:51:05Z

Ok, then I think we should not move this patch forward.

ronag · 2019-07-14T08:25:18Z

@lpinca how about doing check for whether the res has ended? i.e.

if (this.res && this.res.closed) {
	    this.emit('error', new ERR_SOCKET_CLOSED());

As you pointed out the problem occurs here:

node/lib/_http_server.js

Line 615 in ac95c2f

socket.destroySoon();

And we could set closed = true when detaching the socket (i.e. #28621) which would be before that.

Alternatively we could maybe do:

if (this.res && !this.res.socket) {

lpinca · 2019-07-14T08:36:39Z

The problem as I see it is that you don't know the other peer destroyed the socket until you write to it. On the client res.close may not be set and you could still get ECONNRESET.

mcollina

I prefer an implementation that did not overload write and end any further.

mcollina

I'm not in agreement in having this change landed at all. I think It's a perfectly valid case to start receiving a response before we have finished sending the request.

ronag · 2019-07-14T11:17:50Z

I think we are all in agreement about not landing this. I suggest we go back to #27916 and try to find an alternative approach.

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 13, 2019

ronag force-pushed the fix-error-after-response branch 6 times, most recently from b24c8f7 to 74add0b Compare July 13, 2019 12:42

ronag force-pushed the fix-error-after-response branch 6 times, most recently from 8601f70 to 1915854 Compare July 13, 2019 12:50

ronag force-pushed the fix-error-after-response branch from 1915854 to 44c3fef Compare July 13, 2019 13:07

ronag force-pushed the fix-error-after-response branch 2 times, most recently from 8d883c8 to 57fd0b3 Compare July 13, 2019 13:10

lpinca reviewed Jul 13, 2019

View reviewed changes

lib/_http_outgoing.js Outdated Show resolved Hide resolved

ronag force-pushed the fix-error-after-response branch 3 times, most recently from e165a88 to f1ffdb6 Compare July 13, 2019 13:52

ronag force-pushed the fix-error-after-response branch 2 times, most recently from daa197d to 504e6b5 Compare July 13, 2019 16:50

http: don't error after http client response

c5f9627

ronag force-pushed the fix-error-after-response branch from 504e6b5 to c5f9627 Compare July 13, 2019 16:52

lpinca reviewed Jul 13, 2019

View reviewed changes

ronag mentioned this pull request Jul 13, 2019

streams: pipeline stop piping if dest has response #28672

Closed

4 tasks

mcollina requested changes Jul 14, 2019

View reviewed changes

ronag closed this Jul 14, 2019

Uh oh!

http: don't error after http client response #28668

http: don't error after http client response #28668

Uh oh!

Conversation

ronag commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

lpinca commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

benjamingr Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

lpinca Jul 13, 2019

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Jul 13, 2019

Uh oh!

ronag commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Jul 14, 2019

Uh oh!

dougwilson commented Jul 14, 2019

Uh oh!

lpinca commented Jul 14, 2019

Uh oh!

ronag commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ronag commented Jul 13, 2019 •

edited

Loading

ronag commented Jul 13, 2019 •

edited

Loading

lpinca commented Jul 13, 2019 •

edited

Loading

lpinca commented Jul 13, 2019 •

edited

Loading

lpinca commented Jul 13, 2019 •

edited

Loading

ronag commented Jul 13, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading