fix: end message on connection close

[ronag]

If response is not keep alive and socket ends then consider it as end of message.

I'm not sure whether this is spec compliant but I believe it would resolve the issue @Ethan-Arrowood encountered.

Refs: #782

[ronag]

@Ethan-Arrowood if you agree with this would you mind helping with creating a test?

[codecov-commenter]

Codecov Report

Merging #784 (7d938df) into main (9ca6783) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #784      +/-   ##
==========================================
+ Coverage   99.84%   99.87%   +0.02%     
==========================================
  Files          25       25              
  Lines        1942     2335     +393     
==========================================
+ Hits         1939     2332     +393     
  Misses          3        3              

Impacted Files	Coverage Δ
lib/client.js	100.00% <100.00%> (ø)
lib/pool.js	100.00% <0.00%> (ø)
lib/agent.js	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ca6783...7d938df. Read the comment docs.

[ronag]

I actually disagree with this change myself but leaving it here for visibility and discussion.

[Ethan-Arrowood]

Before I can form a real opinion here I'd like to simplify and reproduce. We should try and emulate with just a node.js http server and see what happens. I'm not well versed enough in http spec to know exactly what should happen either.

[mcollina]

I think we should land this change, it's what users would expect.

There is a HTTP/1.1 spec violation that we should document as there is no content-length and no transfer-encoding: chunked specified.

[ronag]

I think we should land this change, it's what users would expect.

Just so we are all in agreement. There is actually (probably?) no way to tell whether we properly received the full body. So in theory the user could receive an incomplete body without error. Are we ok with that?

[ronag]

I think there might be a trailing \r\n in the last packet before the socket end. Maybe llhttp could consider that as end of body for non persistent connections without content-length? @indutny

Also does llhttp error for responses that don't have content-length nor chunked encoding but has connection: keep-alive? @Ethan-Arrowood do you think you could make a test case for that as well?

[dnlup]

Right now, I am not sure about it either. I have read the discussion, and I think this would be necessary only if lhttp doesn't call onMessageComplete in this scenario. I need to see what Node core does in this case. That might give some inspiration.

[dnlup]

I remember having a different behavior with control characters in headers using undici in version 2. Putting \n in a response header caused the connection to be closed abruptly. However, that wasn't happening in Node because there was some validation on the js side that prevented this. Maybe this should be handled outside llhttp.

[mcollina]

I stumbled upon this as well. Can we land it?

[mcollina]

lgtm

[ronag]

I stumbled upon this as well. Can we land it?

I'm not comfortable landing it in it's current form. Would like to see if there is any \r\n we can detect and use.

[mcollina]

I'll investigate further and try to reproduce with Node.

[ronag]

Also would be good to have a separate test for the case where it's keep alive and whether or not that must error.

[ronag]

another thing to consider is whether to only allow this behavior for failure status codes?

[indutny]

Could unidici just call llhttp_finish()?

[mcollina]

@ronag I pushed a test and a fix.

I had to disable another test which is mutually exclusive as far as I understand. I think we should be lenient in this case.

[ronag]

What about llhttp_finish?

[mcollina]

What about llhttp_finish?

I'm investigating it.

[mcollina]

Using llhtp_finish fixed most problems indeed.

[ronag]

LGTM, apart from the comment for the keep-alive test.

[ronag]

LGTM, will make a rc2 release?

[mcollina]

As soon as the test pass! 🎉🎉