fix: reset socket on GET & HEAD with body

[ronag]

Fixes: #246

[mcollina]

lgtm

This probably should be written somewhere in the README as it's more strict compared to Node core.

[ronag]

@mcollina How strongly do you feel in regards to "semversiness" on this?

[mcollina]

If we were crashing with GET and HEAD with a body before, it's a patch.

[ronag]

Added a "notes" section in the docs, documenting this.

[ronag]

@szmarczak

[szmarczak]

I suggest adding an option to disable this. At Got we had bunch of issues complaining about this behavior.

Otherwise looks good :)

[delvedor]

Is this happening only if pipelining is higher than 1? In such case we could allow sending a body if pipeline is 1 and throw it is higher than 1.
What do you think?

[szmarczak]

I agree, but that still doesn't fix the issue.

[ronag]

Is this happening only if pipelining is higher than 1?

It happens when the connection is re-used. Should probably clarify.

This might actually be a problem in Node core as well. Just harder to hit.

[ronag]

An alternative solution is to only send the request when nothing else is running, not send any further requests while it's running, and reconnect once the first response has been received.

[ronag]

@delvedor @mcollina @szmarczak PTAL. I think this might be a better solution, i.e. don't allow any other running requests and reset the socket after the first response.

[mcollina]

lgtm

[szmarczak]

Just a part of my comment from #258 (comment)

IMO, if it looks like a proper response, it should be parsed, and if the path doesn't match with the pipelined request (or there isn't any) then throw reconnect.

What about this?

[ronag]

if the path doesn't match with the pipelined request

I don't think responses contain the path? i.e. at least the http_parser does not provide it.

[szmarczak]

If the data seems legit (looks like a HTTP/1.1 request), then you can check the path (url) here:

https://github.com/mcollina/undici/blob/e1990ce23c51a8b3fa4d4e18570f47467672ee28/lib/client-base.js#L370-L371

And if it doesn't match the pipelined request or there is none just simply reconnect.

[ronag]

If the data seems legit (looks like a HTTP/1.1 request), then you can check the path (url) here:

url is only set for requests, not responses

[szmarczak]

Oooooh I was so confused for a while 😂 You're totally right. Pardon me.