HTTP2 Response Compression/Request Decompression #3126

dimitribouniol · 2024-01-03T18:16:00Z

These changes are now available in 4.92.8

Fixed an issue where HTTP2 didn't support response compression and request decompression.

It seems like it may have been omitted when adding explicit support for HTTP2. Not sure what to do about testing as I couldn't find any tests for the HTTP1.1 pathway, but I did verify it works in my pet project 😅

Fixes #3125

gwynne · 2024-01-03T18:17:23Z

I couldn't tell you why for the life of me, but IIRC this omission was deliberate.

@0xTim @Lukasa Can you provide any insights here?

codecov-commenter · 2024-01-03T18:20:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.28%. Comparing base (11cdb29) to head (ae043ea).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3126      +/-   ##
==========================================
+ Coverage   76.86%   77.28%   +0.41%     
==========================================
  Files         211      211              
  Lines        8119     7713     -406     
==========================================
- Hits         6241     5961     -280     
+ Misses       1878     1752     -126

Files	Coverage Δ
Sources/Vapor/HTTP/Server/HTTPServer.swift	`94.52% <100.00%> (+13.75%)`	⬆️

... and 90 files with indirect coverage changes

0xTim · 2024-01-03T18:35:49Z

Probably just missed when we added compression support. I can't think of anything against it

dimitribouniol · 2024-01-03T18:50:40Z

Linking #2214 which seems to have been when it was modernized, though HTTP2 didn't have any support then either.

dimitribouniol · 2024-02-21T10:14:08Z

Rebased the merge conflict — this has been working well for me over the past few weeks of use, and could really help reduce the amount of data transmitted when large JSON objects are sent out over HTTP/2.

dimitribouniol · 2024-03-29T12:57:25Z

Since it's been a month, figured I'd check in to see if there are still unknowns to figure out here.

…quest recompression Fixes vapor#3125

dimitribouniol · 2024-04-19T10:38:22Z

Tests/VaporTests/ServerTests.swift

+        // TODO: The server should probably reject this?
+        let unsupportedCompressedResponse = try app.client.post("http://localhost:\(port)/compressed") { request in
+            request.headers.replaceOrAdd(name: .contentEncoding, value: "gzip")
+            request.body = ByteBuffer(data: compressedPayload)
+        }.wait()
+
+        if let body = unsupportedCompressedResponse.body {
+            let decodedResponse = try JSONDecoder().decode(TestResponse.self, from: body)
+            XCTAssertEqual(decodedResponse.contentString, String(decoding: compressedPayload, as: UTF8.self))
+            XCTAssertEqual(decodedResponse.contentLength, compressedPayload.count)
+        } else {
+            XCTFail()
+        }


Probably better as a follow-up PR, but a question regarding this currently working behavior: Should this actually be rejected, because the server explicitly doesn't support request decompression for this test case? I can file an issue if there is agreement here.

I would have thought this would be rejected. @0xTim Any thoughts?

Yeah I think the server should reject the request in this case

We should probably return a 415 in this case

gwynne

This looks great to me! @0xTim Can you take a quick look to make sure we didn't miss anything?

…th HTTP 1.1 and HTTP 2

0xTim

Looks good to me! I agree we should reject the request if it's compressed and it's been disabled but I also think we should enable request decompression by default. Happy to discuss this in a new PR though

0xTim · 2024-04-19T20:42:57Z

Tests/VaporTests/ServerTests.swift

+            XCTFail("Missing unsupportedNoncompressedResponse.body")
+        }
+
+        // TODO: The server should probably reject this?


Yeah I agree with this. Though it's not the end of the world. I do think we should probably default to enabled for request decompression, what do you think @gwynne ?

Agreed; as far as I know, there's no reason not to allow it by default, notwithstanding such bugs as swift-server/async-http-client#739

Funnily enough, I wasted probably close to an hour due to that default ratio of 10 wondering why the tests were failing with my test strings. The .size constraint is relative to the decompressed size, so it might make more sense to have that be the default constraint for decompression on both the server and client.

* main: Patch configuration and log actual port on startup (vapor#3160) Update provider tests to 5.10 (vapor#3178) Migrate to Async NIOFileIO APIs (vapor#3167) Removed streamFile deprecation + deactivated advancedETagComparison by default (vapor#3177) Remove HeadResponder (vapor#3147) Advanced ETag Comparison now supported (vapor#3015) Enabled Request Decompression By Default (vapor#3175) HTTP2 Response Compression/Request Decompression (vapor#3126) Don't set ignore status for SIGTERM and SIGINT on Linux (vapor#3174) Fix typos across the codebase (vapor#3162) Fix some Sendable warnings on 5.10 (vapor#3158) Allow `HTTPServer`'s configuration to be dynamically updatable (vapor#3132) Fix issue when client disconnects midway through a stream (vapor#3102) Fix handling of "flag" URL query params (vapor#3151) Bump the dependencies group with 1 update (vapor#3148) Merge Async Tests (vapor#3141) Fix URI handling with multiple slashes and variable components. (vapor#3143) Fix broken URI behaviors (vapor#3140) # Conflicts: # Package.swift

dimitribouniol requested review from 0xTim and gwynne as code owners January 3, 2024 18:16

dimitribouniol force-pushed the dimitri/compression branch from 7d767cd to 6589f3e Compare January 3, 2024 18:16

dimitribouniol force-pushed the dimitri/compression branch 3 times, most recently from 2052970 to 982e424 Compare January 7, 2024 21:41

dimitribouniol force-pushed the dimitri/compression branch 3 times, most recently from 1be7bd3 to c00c07a Compare January 30, 2024 08:37

dimitribouniol force-pushed the dimitri/compression branch 2 times, most recently from ed97e1b to 6fa0bed Compare February 14, 2024 13:06

dimitribouniol force-pushed the dimitri/compression branch from 6fa0bed to 4c00f60 Compare February 21, 2024 10:10

dimitribouniol force-pushed the dimitri/compression branch from 4c00f60 to ee4a762 Compare March 23, 2024 09:01

dimitribouniol force-pushed the dimitri/compression branch from ee4a762 to 25570f0 Compare April 15, 2024 08:53

Fixed an issue where HTTP2 didn't support response compression and re…

c51e073

…quest recompression Fixes vapor#3125

dimitribouniol force-pushed the dimitri/compression branch from 25570f0 to c51e073 Compare April 19, 2024 07:26

dimitribouniol commented Apr 19, 2024

View reviewed changes

dimitribouniol force-pushed the dimitri/compression branch 2 times, most recently from ee93310 to ae043ea Compare April 19, 2024 11:22

gwynne approved these changes Apr 19, 2024

View reviewed changes

dimitribouniol force-pushed the dimitri/compression branch from ae043ea to 6424792 Compare April 19, 2024 11:44

Added exhaustive tests for request and response compression across bo…

aecd8e1

…th HTTP 1.1 and HTTP 2

dimitribouniol force-pushed the dimitri/compression branch from 6424792 to aecd8e1 Compare April 19, 2024 11:46

0xTim added the semver-patch Internal changes only label Apr 19, 2024

0xTim approved these changes Apr 19, 2024

View reviewed changes

0xTim merged commit 71dff4f into vapor:main Apr 19, 2024
16 checks passed

dimitribouniol deleted the dimitri/compression branch April 20, 2024 07:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP2 Response Compression/Request Decompression #3126

HTTP2 Response Compression/Request Decompression #3126

dimitribouniol commented Jan 3, 2024 •

edited by penny-for-vapor bot

gwynne commented Jan 3, 2024

codecov-commenter commented Jan 3, 2024 •

edited by codecov bot

0xTim commented Jan 3, 2024

dimitribouniol commented Jan 3, 2024

dimitribouniol commented Feb 21, 2024

dimitribouniol commented Mar 29, 2024

dimitribouniol Apr 19, 2024 •

edited

gwynne Apr 19, 2024

0xTim Apr 19, 2024

0xTim Apr 19, 2024

gwynne left a comment

0xTim left a comment

0xTim Apr 19, 2024

gwynne Apr 19, 2024

dimitribouniol Apr 19, 2024

HTTP2 Response Compression/Request Decompression #3126

HTTP2 Response Compression/Request Decompression #3126

Conversation

dimitribouniol commented Jan 3, 2024 • edited by penny-for-vapor bot

gwynne commented Jan 3, 2024

codecov-commenter commented Jan 3, 2024 • edited by codecov bot

Codecov Report

0xTim commented Jan 3, 2024

dimitribouniol commented Jan 3, 2024

dimitribouniol commented Feb 21, 2024

dimitribouniol commented Mar 29, 2024

dimitribouniol Apr 19, 2024 • edited

Choose a reason for hiding this comment

gwynne Apr 19, 2024

Choose a reason for hiding this comment

0xTim Apr 19, 2024

Choose a reason for hiding this comment

0xTim Apr 19, 2024

Choose a reason for hiding this comment

gwynne left a comment

Choose a reason for hiding this comment

0xTim left a comment

Choose a reason for hiding this comment

0xTim Apr 19, 2024

Choose a reason for hiding this comment

gwynne Apr 19, 2024

Choose a reason for hiding this comment

dimitribouniol Apr 19, 2024

Choose a reason for hiding this comment

dimitribouniol commented Jan 3, 2024 •

edited by penny-for-vapor bot

codecov-commenter commented Jan 3, 2024 •

edited by codecov bot

dimitribouniol Apr 19, 2024 •

edited