miniflare: compress responses more like Cloudflare FL #5798

RamIdeas · 2024-05-09T16:00:37Z

What this PR solves / how to test

This PR includes the mime-types from the docs and limits miniflare's default compression behaviour to only mime-types that the Cloudflare platform itself compresses

This should make local development more closely match production.

Previous issue comments:

Note for reviewers: I updated the whitespacing within the tagged template literals which were still 2-space rather than tabs, please use the 2nd commit to review which is much more minimal (I've also highlighted the important parts below)

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s): https://github.com/cloudflare/cloudflare-docs/pull/...
- Not necessary because: fix

changeset-bot · 2024-05-09T16:00:40Z

🦋 Changeset detected

Latest commit: e78938d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
miniflare	Patch
@cloudflare/pages-shared	Patch
@cloudflare/vitest-pool-workers	Patch
wrangler	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

RamIdeas · 2024-05-09T16:25:38Z

@mhart @frandiox can you confirm whether or not this fixes your issues please?

github-actions · 2024-05-09T16:34:20Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9033226953/npm-package-wrangler-5798

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5798/npm-package-wrangler-5798

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9033226953/npm-package-wrangler-5798 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9033226953/npm-package-create-cloudflare-5798 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9033226953/npm-package-cloudflare-kv-asset-handler-5798

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9033226953/npm-package-miniflare-5798

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9033226953/npm-package-cloudflare-pages-shared-5798

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9033226953/npm-package-cloudflare-vitest-pool-workers-5798

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.55.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240419.1
`workerd`	1.20240419.0	1.20240419.0
`workerd --version`	1.20240419.0	2024-04-19

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

RamIdeas · 2024-05-09T23:27:26Z

packages/miniflare/test/index.spec.ts

 		});
 		t.is(await res.text(), testBody, encoding);
+		t.is(res.headers.get('Content-Encoding'), encoding, encoding);


I noticed this was test wasn't actually testing all encodings – the encoding was always "gzip". I added this assertion and it was failing – I had to add the "Accept-Encoding" header above so we are now actually testing each encoding

RamIdeas · 2024-05-09T23:31:33Z

packages/miniflare/src/workers/core/entry.worker.ts

+	const contentType = response.headers.get("Content-Type");
+
+    // if cloudflare's FL does not compress this mime-type, then don't compress locally either
+    if (!isCompressedByCloudflareFL(contentType)) {
+        return response;
+    }


This is the main fix, in miniflare's entry worker

RamIdeas · 2024-05-09T23:32:38Z

packages/miniflare/test/index.spec.ts

+	// Check `Content-Type: text/html` is compressed (FL always compresses html)
+	res = await fetch(defaultCompressedUrl);
+	t.is(res.headers.get("Content-Type"), "text/html");
+	t.is(res.headers.get("Content-Encoding"), 'gzip');
+	t.is(await res.text(), testBody);
+
+	// Check `Content-Type: text/event-stream` is not compressed (FL does not compress this mime type)
+	res = await fetch(defaultUncompressedUrl);
+	t.is(res.headers.get("Content-Type"), "text/event-stream");
+	t.is(res.headers.get("Content-Encoding"), null);
+	t.is(await res.text(), testBody);


These were the tests confirmed to fail without the fix, passing with the fix

mhart · 2024-05-10T00:05:55Z

@RamIdeas It works! At least, in this fairly common case of Workers AI use:

const body = await env.AI.run('@cf/meta/llama-2-7b-chat-int8', {
  prompt: 'Say hi',
  stream: true,
});
return new Response(body, { headers: { 'content-type': 'text/event-stream' } });

Tried with some other content types like text/plain and text/html and they do get gzip encoded – which means they also don't stream smoothly (and will continue not to until some workerd changes happen).

So, users will still see issues in use cases like streaming HTML rendering – but I suspect there's nothing more for miniflare to do here. 🚀

frandiox · 2024-05-10T05:03:30Z

can you confirm whether or not this fixes your issues please?

No luck. A simple text/html response would still go through this code path and set the content-encoding.

mf.dispatchFetch(...) returns a decompressed response with content-encoding gzip, so we can't forward it to the browser as is. Since undici's fetch always decompresses the response, perhaps mf.dispatchFetch should remove the content-encoding header?

From the original issue:

Running mf.dispatchFetch(...) (or plain fetch(workerUrl)) is decompressing the response but keeping Content-Encoding: gzip or similar in the response headers. Then, once we forward this response from Node (e.g. Vite server) to the browser, the browser expects encoded content but it is not encoded anymore.

Unfortunately, I think there's no way to avoid decompressing the response in undici. The workaround is to remove Content-Encoding from the Miniflare response or recompressing it in Node -- or perhaps use Node's http.get instead of fetch to get the response... which is not intuitive with mf.dispatchFetch().

Not sure if mf.dispatchFetch is supposed to work in Node proxies. Maybe not and this is not an issue. Perhaps I just need to use undici's request directly to communicate with the worker instead of mf.dispatchFetch, although it feels counter intuitive.

packages/miniflare/src/shared/mime-types.ts

since undici has already decompressed the response body

RamIdeas · 2024-05-10T12:53:47Z

Not sure if mf.dispatchFetch is supposed to work in Node proxies. Maybe not and this is not an issue. Perhaps I just need to use undici's request directly to communicate with the worker instead of mf.dispatchFetch, although it feels counter intuitive.

Ah I thought my change to _transformsForContentEncodingAndContentType handled this case but seems the test uses miniflare in front of a node server, not the other way around.

Unfortunately, I think there's no way to avoid decompressing the response in undici. The workaround is to remove Content-Encoding from the Miniflare response or recompressing it in Node -- or perhaps use Node's http.get instead of fetch to get the response... which is not intuitive with mf.dispatchFetch().

Since you first posted this, I've thought the response to be malformed. If the body is decompressed, the Content-Encoding header shouldn't imply it is compressed. So I agree we should remove the header. I have done in a following commit. Would you mind trying again?

This actually means you will be proxying an uncompressed response. Maybe your proxy will apply its own compression? Not the end of the world locally, but that seems wasteful so, in fact, I think miniflare should use undici.request not undici.fetch but that seemed like a larger change (the options don't match up) so will leave it uncompressed for now.

(Also, thanks for your prompt response!)

penalosa

Approved pending confirmation

frandiox · 2024-05-13T12:29:23Z

So I agree we should remove the header. I have done in a following commit. Would you mind trying again?

Thanks. I've tried adding this commit 660d82e locally and it works well ✅

This actually means you will be proxying an uncompressed response. Maybe your proxy will apply its own compression? Not the end of the world locally, but that seems wasteful so, in fact, I think miniflare should use undici.request not undici.fetch but that seemed like a larger change (the options don't match up) so will leave it uncompressed for now.

Yeah, definitely better uncompressed than broken! Whenever we want compressed responses I'll use undici.request manually.

And I agree Miniflare should probably do that internally but there might be a good reason why it doesn't already 🤔

RamIdeas · 2024-05-13T12:34:01Z

Thanks for confirming. Will merge now and it'll go out in tomorrow's release

RamIdeas added 2 commits May 9, 2024 13:53

chore: fix whitespace (use tabs within tempate literal strings)

8dc6056

fix: compress responses more like Cloudflare FL

874780f

RamIdeas added the e2e label May 9, 2024

add changeset

Loading
Loading status checks…

094e47b

RamIdeas force-pushed the miniflare-compress-like-fl branch from a145d09 to 094e47b Compare May 9, 2024 16:04

RamIdeas marked this pull request as ready for review May 9, 2024 16:21

RamIdeas requested a review from a team as a code owner May 9, 2024 16:21

fix import order

Loading
Loading status checks…

e425172

RamIdeas commented May 9, 2024

View reviewed changes

penalosa approved these changes May 10, 2024

View reviewed changes

packages/miniflare/src/shared/mime-types.ts Outdated Show resolved Hide resolved

packages/miniflare/src/shared/mime-types.ts Outdated Show resolved Hide resolved

RamIdeas added 3 commits May 10, 2024 12:11

nits

6a725c6

chore: refactor undici imports to use namespace

e5c6bbe

fix: remove Content-Encoding header in dispatchFetch

Loading
Loading status checks…

660d82e

since undici has already decompressed the response body

RamIdeas force-pushed the miniflare-compress-like-fl branch from b683d6c to 660d82e Compare May 10, 2024 12:53

RamIdeas requested a review from penalosa May 10, 2024 13:18

penalosa approved these changes May 10, 2024

View reviewed changes

RamIdeas mentioned this pull request May 10, 2024

Set Accept-Encoding: identity for AI API requests #5713

Closed

9 tasks

add test assertions + explanatory comments

Loading
Loading status checks…

e78938d

RamIdeas merged commit 89b6d7f into main May 13, 2024
21 checks passed

RamIdeas deleted the miniflare-compress-like-fl branch May 13, 2024 12:34

workers-devprod mentioned this pull request May 13, 2024

Version Packages #5811

Merged

RamIdeas mentioned this pull request May 13, 2024

🐛 BUG: vercel/ai streaming broken with 3.50.0 in dev | unexpected gzip compression #5614

Closed

penalosa mentioned this pull request Jul 8, 2024

🐛 BUG: net::ERR_CONTENT_DECODING_FAILED on Miniflare fetch #5630

Closed

hi-ogawa mentioned this pull request Jul 9, 2024

chore(deps): update root dev deps hi-ogawa/vite-plugins#523

Merged

3 tasks

CarmenPopoviciu mentioned this pull request Jul 19, 2024

🐛 BUG: #5623

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

miniflare: compress responses more like Cloudflare FL #5798

miniflare: compress responses more like Cloudflare FL #5798

RamIdeas commented May 9, 2024 •

edited by petebacondarwin

Loading

changeset-bot bot commented May 9, 2024 •

edited

Loading

RamIdeas commented May 9, 2024

github-actions bot commented May 9, 2024 •

edited

Loading

RamIdeas May 9, 2024 •

edited

Loading

RamIdeas May 9, 2024

RamIdeas May 9, 2024

mhart commented May 10, 2024 •

edited

Loading

frandiox commented May 10, 2024

RamIdeas commented May 10, 2024 •

edited

Loading

penalosa left a comment

frandiox commented May 13, 2024

RamIdeas commented May 13, 2024

miniflare: compress responses more like Cloudflare FL #5798

miniflare: compress responses more like Cloudflare FL #5798

Conversation

RamIdeas commented May 9, 2024 • edited by petebacondarwin Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented May 9, 2024 • edited Loading

🦋 Changeset detected

RamIdeas commented May 9, 2024

github-actions bot commented May 9, 2024 • edited Loading

RamIdeas May 9, 2024 • edited Loading

Choose a reason for hiding this comment

RamIdeas May 9, 2024

Choose a reason for hiding this comment

RamIdeas May 9, 2024

Choose a reason for hiding this comment

mhart commented May 10, 2024 • edited Loading

frandiox commented May 10, 2024

RamIdeas commented May 10, 2024 • edited Loading

penalosa left a comment

Choose a reason for hiding this comment

frandiox commented May 13, 2024

RamIdeas commented May 13, 2024

RamIdeas commented May 9, 2024 •

edited by petebacondarwin

Loading

changeset-bot bot commented May 9, 2024 •

edited

Loading

github-actions bot commented May 9, 2024 •

edited

Loading

RamIdeas May 9, 2024 •

edited

Loading

mhart commented May 10, 2024 •

edited

Loading

RamIdeas commented May 10, 2024 •

edited

Loading