Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set Content-Length header in server responses #1423

Merged
merged 7 commits into from
Jun 29, 2022

Conversation

hlbarber
Copy link
Contributor

Motivation and Context

Description

  • Add Content-Length header to non-streaming server responses
  • Add a Content-Length checks to the existing httpResponseTests

@hlbarber hlbarber marked this pull request as ready for review June 1, 2022 10:39
@hlbarber hlbarber requested review from a team as code owners June 1, 2022 10:39
@hlbarber
Copy link
Contributor Author

hlbarber commented Jun 1, 2022

Some ambiguity exists on whether this should be left to hyper (which will perform it) or whether we should eagerly add the Content-Length headers at this level.

@github-actions
Copy link

github-actions bot commented Jun 1, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec 4.09% 50176.2 48205.28
Total requests 4.13% 4520878 4341481
Total errors NaN% 0 0
Total successes 4.13% 4520878 4341481
Average latency ms -7.14% 0.65 0.7
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms 2.29% 16.94 16.56
Stdev latency ms -30.91% 0.38 0.55
Transfer Mb 4.13% 469.95 451.3
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@smithy-lang smithy-lang deleted a comment from github-actions bot Jun 1, 2022
Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Linking your findings here for posterity: smithy-lang/smithy#1244.

It'd be nice if we documented somewhere, perhaps in serverRenderContentLengthHeader, all the research you did regarding when Hyper adds/removes Content-Length in HTTP1/HTTP2.

We should contribute a PR to awslabs/smithy adding an @httpResponseTests asserting Content-Length header has the expected value (despite it being a SHOULD in the HTTP RFC, which you can bring up in the PR, because they are already asserting it in @httpRequestTests for clients).

@hlbarber
Copy link
Contributor Author

hlbarber commented Jun 3, 2022

@david-perez

We should contribute a PR to awslabs/smithy adding an @httpResponseTests asserting Content-Length header has the expected value

Posted an issue inquiring about this, will follow with a PR shortly after if needed

smithy-lang/smithy#1255

@github-actions
Copy link

github-actions bot commented Jun 6, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec -3.93% 87562.02 91140.42
Total requests -3.90% 7887516 8207639
Total errors NaN% 0 0
Total successes -3.90% 7887516 8207639
Average latency ms -9.52% 1.14 1.26
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms 0.42% 24.01 23.91
Stdev latency ms -6.70% 2.09 2.24
Transfer Mb -3.90% 819.91 853.19
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@hlbarber hlbarber enabled auto-merge (squash) June 9, 2022 20:20
@github-actions
Copy link

github-actions bot commented Jun 9, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber hlbarber merged commit 5a5aa2f into main Jun 29, 2022
@hlbarber hlbarber deleted the harryb/server-content-length branch June 29, 2022 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set Content-Length where appropiate in server responses
4 participants