Add compression support to ctx.http.fetch - #5698
Open
krisajenkins wants to merge 1 commit into
Open
Conversation
Procedures making outbound HTTP requests via `ctx.http.fetch` (TypeScript) or
`ctx.http.send`/`get` (Rust) never negotiated compression. The workspace
`reqwest` dependency enabled neither the `gzip` nor the `brotli` feature, so the
decompression code was not compiled in at all, and requests went out with no
`Accept-Encoding` header:
GET / HTTP/1.1
accept: */*
host: 127.0.0.1:56586
Every response body was therefore transferred uncompressed. A module that wanted
compression had to set `Accept-Encoding` by hand and inflate the body itself.
That is wasted bandwidth and latency on every procedure call to a JSON or text
API — which is most of them — and it counts against the 30s default and 180s
maximum request timeouts.
Enable reqwest's `gzip` and `brotli` features. reqwest then advertises
`Accept-Encoding: gzip, br` and decodes response bodies transparently, so the
client builder in `InstanceEnv::http_request` needs no change and no module code
has to change to benefit. `brotli` adds little on top of `gzip`: enabling `gzip`
pulls in `async-compression` regardless, and the `brotli` crate is already
compiled into `spacetimedb-core` for WebSocket compression.
Note that feature unification applies this to every workspace crate using
`reqwest`, not only the procedure host. The one place that decodes a response
body by hand is `crates/update`, which gunzips downloaded release tarballs; that
path keys off `Content-Type: application/gzip` rather than `Content-Encoding`,
so it is unaffected.
# API and ABI breaking changes
No API or ABI change, but there is a minor behavioural break for modules that
worked around the missing support by setting `Accept-Encoding: gzip` themselves
and inflating the response body in module code. reqwest leaves a caller-supplied
`Accept-Encoding` header as-is, but still decodes the response, so those modules
now receive plaintext and their own inflate step will fail on it. The fix is to
delete the manual decompression. This is pinned by a test so it does not get
rediscovered later.
Three smaller observable changes on responses the server chose to compress:
- `Content-Encoding` and `Content-Length` are stripped once the body is decoded,
as both describe bytes that no longer exist.
- There is no longer any way to obtain the raw compressed bytes.
- `procedure_http_response_size_bytes` now counts decompressed bytes rather than
bytes on the wire.
# Expected complexity level and risk
2 — the change itself is two words in a dependency's feature list. The risk is
not in the diff but in its reach: feature unification applies it to every
workspace crate using `reqwest`, and it silently changes the bytes every
procedure sees on a compressed response.
# Testing
Added three tests to `crates/core/src/host/instance_env.rs`. Each stands up a
one-shot loopback HTTP server and drives a real `InstanceEnv::http_request` —
the same path `ctx.http.fetch` takes — rather than testing reqwest in isolation.
They are gated on `allow_loopback_http_for_tests`, like the existing egress
tests.
- [x] A `Content-Encoding: gzip` response body reaches the module decompressed
- [x] A `Content-Encoding: br` response body reaches the module decompressed
- [x] `Content-Encoding` is stripped from the response headers after decoding
- [x] A caller-supplied `Accept-Encoding` is sent unmodified and the body is
decoded anyway, pinning the breaking change described above
- [x] Confirmed the tests are not vacuous: reverting the `Cargo.toml` change
fails all three, with no `Accept-Encoding` on the outgoing request
- [x] `cargo clippy -p spacetimedb-core --all-targets` clean; `cargo check`
passes for `cli`, `standalone`, `update` and `guard`
- [ ] Reviewer: sanity-check that the `crates/update` tarball download is
genuinely unaffected, ideally against the real release CDN
- [ ] Reviewer: confirm you are happy with the workspace-wide blast radius, or
ask for the features to be scoped more narrowly
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If you make an outbound HTTP requests (via
ctx.http.fetch(TypeScript) orctx.http.send/get(Rust)) you don't get compression. But it's surprisinglyeasy to add, and obviously a huge benefit to transfer sizes/times.
Details
The workspace
reqwestdependency enabled neither thegzipnor thebrotlifeature, so the decompression code was not compiled in at all, and requests
went out with no
Accept-Encodingheader:Every response body was therefore transferred uncompressed. A module that wanted
compression had to set
Accept-Encodingby hand and inflate the body itself.That is wasted bandwidth and latency on every procedure call to a JSON or text
API — which is most of them — and it counts against the 30s default and 180s
maximum request timeouts.
Enable reqwest's
gzipandbrotlifeatures. reqwest then advertisesAccept-Encoding: gzip, brand decodes response bodies transparently, so theclient builder in
InstanceEnv::http_requestneeds no change and no module codehas to change to benefit.
brotliadds little on top ofgzip: enablinggzippulls in
async-compressionregardless, and thebrotlicrate is alreadycompiled into
spacetimedb-corefor WebSocket compression.Note that feature unification applies this to every workspace crate using
reqwest, not only the procedure host. The one place that decodes a responsebody by hand is
crates/update, which gunzips downloaded release tarballs; thatpath keys off
Content-Type: application/gziprather thanContent-Encoding,so it is unaffected.
API and ABI breaking changes
No API or ABI change, but there is a minor behavioural break for modules that
worked around the missing support by setting
Accept-Encoding: gzipthemselvesand inflating the response body in module code. reqwest leaves a caller-supplied
Accept-Encodingheader as-is, but still decodes the response, so those modulesnow receive plaintext and their own inflate step will fail on it. The fix is to
delete the manual decompression. This is pinned by a test so it does not get
rediscovered later.
Three smaller observable changes on responses the server chose to compress:
Content-EncodingandContent-Lengthare stripped once the body is decoded,as both describe bytes that no longer exist.
procedure_http_response_size_bytesnow counts decompressed bytes rather thanbytes on the wire.
Expected complexity level and risk
2 — the change itself is two words in a dependency's feature list. The risk is
not in the diff but in its reach: feature unification applies it to every
workspace crate using
reqwest, and it silently changes the bytes everyprocedure sees on a compressed response.
Testing
Added three tests to
crates/core/src/host/instance_env.rs. Each stands up aone-shot loopback HTTP server and drives a real
InstanceEnv::http_request—the same path
ctx.http.fetchtakes — rather than testing reqwest in isolation.They are gated on
allow_loopback_http_for_tests, like the existing egresstests.
Content-Encoding: gzipresponse body reaches the module decompressedContent-Encoding: brresponse body reaches the module decompressedContent-Encodingis stripped from the response headers after decodingAccept-Encodingis sent unmodified and the body isdecoded anyway, pinning the breaking change described above
Cargo.tomlchangefails all three, with no
Accept-Encodingon the outgoing requestcargo clippy -p spacetimedb-core --all-targetsclean;cargo checkpasses for
cli,standalone,updateandguardcrates/updatetarball download isgenuinely unaffected, ideally against the real release CDN
ask for the features to be scoped more narrowly