quic-go icon indicating copy to clipboard operation
quic-go copied to clipboard

http3: avoid re-parsing of the Content-Length header for responses

Open marten-seemann opened this issue 1 year ago • 6 comments

There's a slight change in behavior here: For requests that don't have a body, we now still set the http.Response.ContentLength to -1 (once we have #4645). I think this ok:

// ContentLength records the length of the associated content. The
// value -1 indicates that the length is unknown. Unless Request.Method
// is "HEAD", values >= 0 indicate that the given number of bytes may
// be read from Body.
ContentLength int

@bt90 @WeidiDeng What do you think?

marten-seemann avatar Aug 29 '24 13:08 marten-seemann

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.94%. Comparing base (b92bf0c) to head (cb82af2). Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4648   +/-   ##
=======================================
  Coverage   84.94%   84.94%           
=======================================
  Files         150      150           
  Lines       14930    14927    -3     
=======================================
- Hits        12681    12679    -2     
+ Misses       1722     1721    -1     
  Partials      527      527           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 29 '24 13:08 codecov[bot]

Talking about the principle of least surprise: if there's no body, I'd be surprised if the length was marked as unknown. A missing body has a known length of 0 bytes. Or is there a reason why we need to distinguish between no body and an empty body?

bt90 avatar Aug 29 '24 13:08 bt90

It just means we can't remove the lines I'm removing here in this PR. I'll update it.

marten-seemann avatar Aug 29 '24 13:08 marten-seemann

It's in fd04f71. It's kind of ugly.

marten-seemann avatar Aug 29 '24 13:08 marten-seemann

I haven't looked into it, but RFC7230 has largely been superseded by RFC9110.

https://datatracker.ietf.org/doc/html/rfc9110#name-content-length

bt90 avatar Aug 29 '24 13:08 bt90

It's in fd04f71. It's kind of ugly.

Ah, i see. So there are three cases:

  • no Content-Length header: either the size is unknown or it's not allowed for this kind of response
  • Content-Length: 0 for an empty body
  • Content-Length: x if a body of known length is present

I'd agree that the -1 value is probably the cleanest solution.

bt90 avatar Aug 29 '24 13:08 bt90

It's in fd04f71. It's kind of ugly.

This commit was slightly wrong, I messed up inverting the boolean expression. It's still not pretty, but it's not worse than what we had before, so I'm inclined to just merge it as is.

marten-seemann avatar Sep 08 '24 07:09 marten-seemann