-
Notifications
You must be signed in to change notification settings - Fork 645
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
304 Not Modified Broken with Response Compression Enabled #2737
Conversation
Great find! This looks like a correct fix, but we'll need some unit tests for this to ensure that we don't regress the functionality. Do you mind adding some? We have some existing no-body tests that could be extended for this use-case. |
Compression test available here: |
@swift-server-bot add to allowlist |
9611cb9
to
ebd749b
Compare
…compression were used and a 304 Not Modified response was returned
ebd749b
to
95fe7c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice one @dimitribouniol, thanks!
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [vapor/vapor](https://togithub.com/vapor/vapor) | minor | `4.93.1` -> `4.102.0` | --- ### Release Notes <details> <summary>vapor/vapor (vapor/vapor)</summary> ### [`v4.102.0`](https://togithub.com/vapor/vapor/releases/tag/4.102.0): - Add Sendable conformance to XCTApplicationTester, XCTHTTPRequest/Response, and some others [Compare Source](https://togithub.com/vapor/vapor/compare/4.101.4...4.102.0) #### What's Changed Add Sendable conformance to XCTApplicationTester, XCTHTTPRequest/Response, and some others by [@​gwynne](https://togithub.com/gwynne) in [#​3208](https://togithub.com/vapor/vapor/issues/3208) > This prevents compiler errors in the Swift 6 language mode for test methods isolated to global actors (especially `@MainActor`). Also adds `Sendable` conformance to `XCTHTTPRequest` and `XCTHTTPResponse`, for much the same reason. > > Also fixes lots and lots of various `Sendable` warnings in Vapor in general, including adding `Sendable` conformance to `ContentConfiguration`, `ContentEncoder`, `ContentDecoder`, `URLQueryEncoder`, `URLQueryDecoder`, `URLEncodedFormEncoder`, and `URLEncodedFormDecoder`. ###### *This patch was released by [@​gwynne](https://togithub.com/gwynne)* **Full Changelog**: vapor/vapor@4.101.4...4.102.0 ### [`v4.101.4`](https://togithub.com/vapor/vapor/releases/tag/4.101.4): - Fixed an issue where response compression would fail when returning 304 Not Modified [Compare Source](https://togithub.com/vapor/vapor/compare/4.101.3...4.101.4) #### What's Changed Fixed an issue where response compression would fail when returning 304 Not Modified by [@​dimitribouniol](https://togithub.com/dimitribouniol) in [#​3206](https://togithub.com/vapor/vapor/issues/3206) > I recently discovered that when response compression was enabled, browsers would fail to load resources they had cached when the server responded with `304 Not Modified`, indicating the browser should use the resource they have, but failed to do so leading to images or stylesheets not loading. This applied to both HTTP 1.1 and 2 servers, using the FileMiddleware to provide the caching logic. This was [fixed](https://togithub.com/apple/swift-nio/pull/2737) in `SwiftNIO`, and has new [tests](https://togithub.com/apple/swift-nio-extras/pull/224) in `NIOHTTPCompression`, so this PR just bumps the minimum version to ensure folks don’t run into the bug if response compression is enabled and caching is actually used. > > ### Learn More: > > - [Discussion on Discord](https://discord.com/channels/431917998102675485/1249304655863877637/1249304655863877637) > - [apple/swift-nio#2737](https://togithub.com/apple/swift-nio/pull/2737) > - [apple/swift-nio-extras#224](https://togithub.com/apple/swift-nio-extras/pull/224) ###### *This patch was released by [@​gwynne](https://togithub.com/gwynne)* **Full Changelog**: vapor/vapor@4.101.3...4.101.4 ### [`v4.101.3`](https://togithub.com/vapor/vapor/releases/tag/4.101.3): - Fix decoding 'flag' URL query params via `.decode(StructType.self)` [Compare Source](https://togithub.com/vapor/vapor/compare/4.101.2...4.101.3) #### What's Changed Fix decoding 'flag' URL query params via `.decode(StructType.self)` by [@​challfry](https://togithub.com/challfry) in [#​3164](https://togithub.com/vapor/vapor/issues/3164) > Fixes [#​3163](https://togithub.com/vapor/vapor/issues/3163). > > The code: > > struct QueryStruct: Content { > var flag1: Bool? > } > let queryStruct = try req.query.decode(QueryStruct.self) > > produces `queryStruct.flag1 == true` when decoding the URL query “?flag1”, matching the behavior of `req.query[Bool.self, at: "flag1"]`. #### New Contributor - [@​challfry](https://togithub.com/challfry) made their first contribution in [#​3164](https://togithub.com/vapor/vapor/issues/3164) 🎉 ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.101.2...4.101.3 ### [`v4.101.2`](https://togithub.com/vapor/vapor/releases/tag/4.101.2): - Adds TIFF and WebP HTTP Media Types [Compare Source](https://togithub.com/vapor/vapor/compare/4.101.1...4.101.2) #### What's Changed Adds TIFF and WebP HTTP Media Types by [@​qalandarov](https://togithub.com/qalandarov) in [#​3194](https://togithub.com/vapor/vapor/issues/3194) > Add additional image types: > > app.post("upload") { req in > guard [.jpeg, .png, .tiff, .webp].contains(req.content.contentType) else { > throw Abort(.unsupportedMediaType) > } > // ... > } ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.101.1...4.101.2 ### [`v4.101.1`](https://togithub.com/vapor/vapor/releases/tag/4.101.1): - Exclude Query and Fragment from URI semicolon fix on Linux [Compare Source](https://togithub.com/vapor/vapor/compare/4.101.0...4.101.1) On Linux, `URLComponents` does not have 100% the same behavior like on macOS. Vapor accounts for [this unfixed bug](https://togithub.com/apple/swift-corelibs-foundation/issues/3352) by replacing percent-encoded semicolon `%3B` with `;` in `URI`s. This is however not fully correct, because if a URI contains a percent encoded semicolon, this might have a different meaning, than when it is not percent encoded, compare the following sentence from RFC 3986: A percent-encoding mechanism is used to represent a data octet in a component when that octet's corresponding character is outside the allowed set or is being used as a delimiter of, or within, the component. This PR aims to limit the impact of the required semicolon fix by ensuring that query and fragments are not unnecessarily and incorrectly modified. Hopefully, in a future with the new swift-foundation this fix will not be needed anymore. But for now it would solve an issue on our side which is related to the concept of a signed request. ### [`v4.101.0`](https://togithub.com/vapor/vapor/releases/tag/4.101.0): - Add Async Storage shutdown [Compare Source](https://togithub.com/vapor/vapor/compare/4.100.2...4.101.0) #### What's Changed Add Async Storage shutdown by [@​0xTim](https://togithub.com/0xTim) in [#​3196](https://togithub.com/vapor/vapor/issues/3196) > Currently running > > ```swift > Task { > try? await Task.sleep(for: .seconds(5)) > app.running?.stop() > } > ``` > > When you try and install NIO as the global executor will crash because the storage API didn’t have any async entry points so stopping would trigger a synchronous shutdown, with a `wait()`. This fixes that ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.100.2...4.101.0 ### [`v4.100.2`](https://togithub.com/vapor/vapor/releases/tag/4.100.2): - asyncBoot will no longer try booting server again if it is already booted [Compare Source](https://togithub.com/vapor/vapor/compare/4.100.1...4.100.2) #### What's Changed asyncBoot will no longer try booting server again if it is already booted by [@​RussBaz](https://togithub.com/RussBaz) in [#​3195](https://togithub.com/vapor/vapor/issues/3195) > The synchronous `boot` function skips running the lifecycle handlers if the server is already booted. However, the async version ignored this check. I have added a small fix to add this check again. ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.100.1...4.100.2 ### [`v4.100.1`](https://togithub.com/vapor/vapor/releases/tag/4.100.1): - Update URLEncodedFormEncoder encoding rules [Compare Source](https://togithub.com/vapor/vapor/compare/4.100.0...4.100.1) #### What's Changed Update URLEncodedFormEncoder encoding rules by [@​ptoffy](https://togithub.com/ptoffy) in [#​3192](https://togithub.com/vapor/vapor/issues/3192) > Solves [#​3173](https://togithub.com/vapor/vapor/issues/3173) > References https://url.spec.whatwg.org/#application-x-www-form-urlencoded-percent-encode-set to set the encoding rules > > > The [application/x-www-form-urlencoded percent-encode set](https://url.spec.whatwg.org/#application-x-www-form-urlencoded-percent-encode-set) contains all code points, except the [ASCII alphanumeric](https://infra.spec.whatwg.org/#ascii-alphanumeric), U+002A (\*), U+002D (-), U+002E (.), and U+005F (\_). ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.100.0...4.100.1 ### [`v4.100.0`](https://togithub.com/vapor/vapor/releases/tag/4.100.0): - Add Async Lifecycle Handlers [Compare Source](https://togithub.com/vapor/vapor/compare/4.99.3...4.100.0) #### What's Changed Add Async Lifecycle Handlers by [@​0xTim](https://togithub.com/0xTim) in [#​3193](https://togithub.com/vapor/vapor/issues/3193) > Adds new protocol functions to `LifecycleHandler`s to support async contexts. This is important because packages like [Redis](https://togithub.com/vapor/redis) use this to know when to shutdown their connection pool. In the shutdown function, these call `.wait()` which can cause application crashes if called when trying to use NIO’s event loop concurrency executor. > > This provides async alternatives to allow packages to provide full async calls through their stack to avoid these crashes ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.99.3...4.100.0 ### [`v4.99.3`](https://togithub.com/vapor/vapor/releases/tag/4.99.3): - Async Serve Command [Compare Source](https://togithub.com/vapor/vapor/compare/4.99.2...4.99.3) #### What's Changed Async Serve Command by [@​0xTim](https://togithub.com/0xTim) in [#​3190](https://togithub.com/vapor/vapor/issues/3190) > Migrate `ServeCommand` to an `AsyncCommand` to enable proper custom executor support and remove any calls to `wait()` ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.99.2...4.99.3 ### [`v4.99.2`](https://togithub.com/vapor/vapor/releases/tag/4.99.2): - Support compiling against Musl [Compare Source](https://togithub.com/vapor/vapor/compare/4.99.1...4.99.2) #### What's Changed Support compiling against Musl by [@​simonjbeaumont](https://togithub.com/simonjbeaumont) in [#​3188](https://togithub.com/vapor/vapor/issues/3188) > Vapor already makes some provision for compiling against Musl in the RFC1123 implementation, where `Glibc` is not assumed and is imported conditionally alongside a conditional import of `Musl`. However, there are a couple of other places where `Glibc` is still assumed when compiling for Linux. > > This patch replaces these imports with the same `#if canImport(...)` pattern. ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.99.1...4.99.2 ### [`v4.99.1`](https://togithub.com/vapor/vapor/releases/tag/4.99.1): - Fix availability message [Compare Source](https://togithub.com/vapor/vapor/compare/4.99.0...4.99.1) #### What's Changed Fix availability message by [@​valeriyvan](https://togithub.com/valeriyvan) in [#​3191](https://togithub.com/vapor/vapor/issues/3191) > ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.99.0...4.99.1 ### [`v4.99.0`](https://togithub.com/vapor/vapor/releases/tag/4.99.0): - Add async alternative for Application.shutdown [Compare Source](https://togithub.com/vapor/vapor/compare/4.98.0...4.99.0) #### What's Changed Add async alternative for Application.shutdown by [@​0xTim](https://togithub.com/0xTim) in [#​3189](https://togithub.com/vapor/vapor/issues/3189) > Adds an async alternative for `Application.shutdown()` and annotates `shutdown()` with `noasync` ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.98.0...4.99.0 ### [`v4.98.0`](https://togithub.com/vapor/vapor/releases/tag/4.98.0): - Mark all functions that use `wait` as `noasync` [Compare Source](https://togithub.com/vapor/vapor/compare/4.97.1...4.98.0) #### What's Changed Mark all functions that use `wait` as `noasync` by [@​0xTim](https://togithub.com/0xTim) in [#​3168](https://togithub.com/vapor/vapor/issues/3168) >⚠️ **WARNING**: If you have strict concurrency checking enabled you should migrate to the async `Application.make()` > > NIO’s `EventLoopFuture.wait()` is marked as `noasync` because is can cause issues when used in a concurrency context. All places where we call `.wait()` should also be marked as `noasync` to avoid this issue. > > This adds `async` alternatives for those functions and adds `noasync` annotations where appropriate. > > Also adds an `async` `Application.make` to replace the old initialiser that is now `noasync` ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.97.1...4.98.0 ### [`v4.97.1`](https://togithub.com/vapor/vapor/releases/tag/4.97.1): - Log source file and line info for errors in ErrorMiddleware when possible [Compare Source](https://togithub.com/vapor/vapor/compare/4.97.0...4.97.1) #### What's Changed Log source file and line info for errors in ErrorMiddleware when possible by [@​gwynne](https://togithub.com/gwynne) in [#​3187](https://togithub.com/vapor/vapor/issues/3187) > Ever since the last changes to `ErrorMiddleware` (by me, naturally), the error logging fails to correctly report file/line/function information even when the error has that data available. We now correctly pass these along to the logging machinery. The error responses sent to clients are unchanged. > > Additional changes: > > - Restore recognition of the `DebuggableError` protocol (reason and source location information for such errors are now used again). > - Handle generating error responses slightly more efficiently. > - Include the original error message in the fallback text if encoding an error to JSON fails. > - Improve the correctness of the `reason` messages used for `DecodingError`s. ###### *This patch was released by [@​gwynne](https://togithub.com/gwynne)* **Full Changelog**: vapor/vapor@4.97.0...4.97.1 ### [`v4.97.0`](https://togithub.com/vapor/vapor/releases/tag/4.97.0): - Provide AsyncFileStreaming API [Compare Source](https://togithub.com/vapor/vapor/compare/4.96.0...4.97.0) #### What's Changed Provide AsyncFileStreaming API by [@​0xTim](https://togithub.com/0xTim) in [#​3184](https://togithub.com/vapor/vapor/issues/3184) > Builds on the work of [#​2998](https://togithub.com/vapor/vapor/issues/2998), [#​3170](https://togithub.com/vapor/vapor/issues/3170) and [#​3167](https://togithub.com/vapor/vapor/issues/3167) to provide a full async streaming API that can be used in Swift Concurrency environments: > > - Provides a new `asyncStreamFile(at:chunkSize:mediaType:advancedETagComparison:onCompleted:)` that takes advantage of the full async response streaming > - Fixes a number of bugs with the async Response body streaming > - `FileMiddleware` is now an `AsyncMiddleware` > - Replaces usages of `FileManager` with `NIOFileSystem` apart from in one deprecated API that can’t be async > - Correctly marks `XCTVapor` functions as `noasync` where they use `.wait()` and provides proper async alternatives ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.96.0...4.97.0 ### [`v4.96.0`](https://togithub.com/vapor/vapor/releases/tag/4.96.0): - Make # of connections accepted per event loop cycle configurable, and raise the default [Compare Source](https://togithub.com/vapor/vapor/compare/4.95.0...4.96.0) #### What's Changed Make # of connections accepted per event loop cycle configurable, and raise the default by [@​gwynne](https://togithub.com/gwynne) in [#​3186](https://togithub.com/vapor/vapor/issues/3186) > As per [@​weissi](https://togithub.com/weissi)’s suggestion in [this forums post](https://forums.swift.org/t/standard-vapor-website-drops-1-5-of-requests-even-at-concurrency-of-100/71583/49), we raise the default maximum number of connections accepted per cycle of the server’s event loop from 4 to 256, and the value is now user-configurable. > > There are no new tests for this because I’m not sure if there’s a way to measure the effect of changing this value that doesn’t involve nondeterministic timing measurements. > > Also takes the opportunity/excuse to add the missing `customCertificateVerifyCallback` parameter to the initializers of `HTTPServer.Configuration`. #### Reviewers Thanks to the reviewers for their help: - [@​weissi](https://togithub.com/weissi) - [@​MahdiBM](https://togithub.com/MahdiBM) ###### *This patch was released by [@​gwynne](https://togithub.com/gwynne)* **Full Changelog**: vapor/vapor@4.95.0...4.96.0 ### [`v4.95.0`](https://togithub.com/vapor/vapor/releases/tag/4.95.0): - Add support for asynchronous body stream writing [Compare Source](https://togithub.com/vapor/vapor/compare/4.94.1...4.95.0) #### What's Changed Add support for asynchronous body stream writing by [@​Joannis](https://togithub.com/Joannis) in [#​2998](https://togithub.com/vapor/vapor/issues/2998) > - Fixes [#​2930](https://togithub.com/vapor/vapor/issues/2930) - a crash when users try to write a body from within a task towards the ELF APIs. > - Introduces a new API for writing chunked HTTP response bodies > - Adds a helper that automatically manages failing and closing streams ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.94.1...4.95.0 ### [`v4.94.1`](https://togithub.com/vapor/vapor/releases/tag/4.94.1): - Patch configuration and log actual port on startup [Compare Source](https://togithub.com/vapor/vapor/compare/4.94.0...4.94.1) #### What's Changed Patch configuration and log actual port on startup by [@​bisgardo](https://togithub.com/bisgardo) in [#​3160](https://togithub.com/vapor/vapor/issues/3160) > Before this change, the application > > ```swift > let app = Application(.testing) > defer { app.shutdown() } > try app.server.start(hostname: nil, port: 0) > defer { app.server.shutdown() } > ``` > > would log the following message *before* starting the server: > > [Vapor] Server starting on http://127.0.0.1:0 > > After this change it instead logs a message like the following *after* starting the server: > > [Vapor] Server starting on http://127.0.0.1:57935 > > The input configuration is also patched such that `app.http.server.configuration.port` will hold the actual port after startup. Currently if it has value 0 it will keep that value (only `app.http.server.shared.localAddress?.port` will have the correct one). > > Fixes [#​3159](https://togithub.com/vapor/vapor/issues/3159). #### Reviewers Thanks to the reviewers for their help: - [@​dimitribouniol](https://togithub.com/dimitribouniol) ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.94.0...4.94.1 ### [`v4.94.0`](https://togithub.com/vapor/vapor/releases/tag/4.94.0): - Migrate to Async NIOFileIO APIs [Compare Source](https://togithub.com/vapor/vapor/compare/4.93.2...4.94.0) #### What's Changed Migrate to Async NIOFileIO APIs by [@​0xTim](https://togithub.com/0xTim) in [#​3167](https://togithub.com/vapor/vapor/issues/3167) > This migrates `collectFile(at:)` and `writeFile(_:at:)` to use NIO’s async NIOFileIO APIs introduced in https://github.com/apple/swift-nio/releases/tag/2.63.0 > > Also adds a new API for streaming files using a `AsyncSequence` based on the new `NIOFileSystem`. > > This work is required to move the `DotEnv` support over to an async API to avoid calling `wait()`s in an async context which can cause issues #### Reviewers Thanks to the reviewers for their help: - [@​ptoffy](https://togithub.com/ptoffy) ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.93.2...4.94.0 ### [`v4.93.2`](https://togithub.com/vapor/vapor/releases/tag/4.93.2): - Removed streamFile deprecation + deactivated advancedETagComparison by default [Compare Source](https://togithub.com/vapor/vapor/compare/4.93.1...4.93.2) #### What's Changed Removed streamFile deprecation + deactivated advancedETagComparison by default by [@​linus-hologram](https://togithub.com/linus-hologram) in [#​3177](https://togithub.com/vapor/vapor/issues/3177) > As discussed on Discord, this PR removes the deprecation and deactivates the lately introduced advanced ETag Comparison for the time being while the revised implementation is worked on. #### New Contributor - [@​linus-hologram](https://togithub.com/linus-hologram) made their first contribution in [#​3177](https://togithub.com/vapor/vapor/issues/3177) 🎉 ###### *This patch was released by [@​0xTim](https://togithub.com/0xTim)* **Full Changelog**: vapor/vapor@4.93.1...4.93.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDkuNCIsInVwZGF0ZWRJblZlciI6IjM2LjEwOS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
* commit 'fc79798d5a150d61361a27ce0c51169b889e23de': NIOSendableBox: allow off-loop initialisation iff Value is Sendable (apple#2753) Throw an appropriate error from the writer when the channel closed (apple#2744) put snippet code inside @available function (apple#2750) fix link to NIOFileSystem from NIO index page (apple#2747) convert the NIOFileSystem example code to a Snippet (apple#2746) Silence warning about missing include in macOS builds (apple#2741) Correctly mark 304 as not having a response body (apple#2737) Update availability guard (apple#2739) Add API for setting last accessed and last modified file times (apple#2735) Add a fallback path if renameat2 fails (apple#2733) Release file handles back to caller on failure to take ownership (apple#2715) Add a version of 'write' for 'ByteBuffer' (apple#2730) Imrprove rename error (apple#2731) Remove storage indirection for FileSystemError (apple#2726) testSimpleMPTCP should not fail for ENOPROTOOPT (apple#2725) Fix race in TCPThroughputBenchmark (apple#2724)
Fixed an issue where HTTP/2 connections would disconnect if response compression were used and a 304 Not Modified response was returned.
Motivation:
I discovered in the Vapor Discord that initially, Safari was having trouble loading content from my site https://jiiiii.moe, which uses both HTTP/2 and response compression. Loads seemed to be sporadic at best, dropping images and other resources seemingly randomly.
We discovered that this was only ever an issue for 304 Not Modified responses after we were able to reproduce with Chromium, which surfaced an issue understanding the HTTP/2 traffic, along with the headers it attempted and received:
Note that vapor not NIO ever logged any issues even with .trace logging enabled.
Vapor configuration:
To reproduce in Safari:
To reproduce in Chromium:
(I could not get Chromium to ask for them sooner, so the Safari steps are more consistent to trigger the bug)
For what its worth, cURL doesn't seem to have any issues:
--
After these hints, I did more digging and found that HTTPResponseCompressor seemed to be skipping over this early exit:
Which seemed to cause an empty body to become 8 bytes before Chromium (and Safari) gave up trying to decompress and reported failure.
(It predictably skips straight to this end state in the compressor:)
Modifications:
In as straightforward as a fix as I could think of, I added a new case to
mayHaveResponseBody
to returnfalse
when the response code is 304 Not Modified, as this is prohibited by the spec anyways:Ideally, we should follow up with a more holistic fix to skip compressing anything if the response has an empty body, but that should be necessary with the above fix in place.
Result:
With this fix in place, my server now returns 304 responses properly 🎉
More Info:
This investigation has been recorded to two separate live streams for posterity, as that's when I discovered the issue and the fix 😅