@benjamingr ping

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).

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.

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.

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

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

@lpinca how about this version?

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

@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))

@lpinca another try...

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.

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

All data is actually discarded

if the user is not consuming the data.

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

Works for me.

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.

What does the spec say?

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.

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

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

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.

@lpinca updated according to our discussion

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.

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.

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.

@dougwilson

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.

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

@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:

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) {

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.

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.