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

fix(polkompress): avoid hanging requests and/or runtime errors #648

Merged
merged 4 commits into from
May 27, 2021

Conversation

lukeed
Copy link
Member

@lukeed lukeed commented May 27, 2021

Ok, this one is a little long-winded:

The polkompress middleware didn't always "cleanup"/terminate the response correctly. Basically, our monkey-patched res.writeHead held a dangling pendingStatus value for when start() was done. However, this only worked if there was an explicit call to res.writeHead somewhere in the application before the start() exits.

I don't know if this just has to do with the fact that my test case used a small body (even when crossing the threshold) , but in my reproductions I saw that this was always the order:

INSIDE END // via res.end patch
START END { pendingStatus: undefined } // last line of start()
INSIDE WRITEHEAD 200 // via res.writeHead patch

So, the res.writeHead call that res.end makes internally (native, not patched behavior) happens after start() has already finished. This problem manifested as two different errors:

  1. When body was shorter than the threshold, it results in a hanging request that never terminates – at least until timeout takes over

  2. When the body crosses the threshold (and does not take a while to finish compressing, I guess), this nasty error surfaces, which was seen in production (repro below):

Screen_Shot_2021-05-26_at_8 27 29_PM

You can get to both errors with this snippet & by playing with the res.end contents:

http.createServer((req, res) => {
  compression({ level: 1, threshold: 4 })(req, res, () => {
    res.end("OK");
    // res.end("HELLO WORLD");
  })
}).listen(3000);

I added tests (nearly crashed my computer, twice!!) for both variants of this error.

@changeset-bot
Copy link

changeset-bot bot commented May 27, 2021

🦋 Changeset detected

Latest commit: 7345e89

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

This PR includes changesets to release 1 package
Name Type
wmr 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

@github-actions
Copy link
Contributor

github-actions bot commented May 27, 2021

Size Change: +2 B (0%)

Total Size: 759 kB

Filename Size Change
packages/wmr/wmr.cjs 723 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
examples/demo/dist/about/index.html 712 B 0 B
examples/demo/dist/alias-outside/index.html 692 B 0 B
examples/demo/dist/assets/Calendar.********.css 702 B 0 B
examples/demo/dist/assets/style.********.css 624 B 0 B
examples/demo/dist/chunks/alias-outside.********.js 137 B 0 B
examples/demo/dist/chunks/class-fields.********.js 211 B 0 B
examples/demo/dist/chunks/compat.********.js 15.4 kB 0 B
examples/demo/dist/chunks/hoofd.module.********.js 1.46 kB 0 B
examples/demo/dist/chunks/index.********.js 304 B 0 B
examples/demo/dist/chunks/json.********.js 237 B 0 B
examples/demo/dist/chunks/meta-tags.********.js 299 B 0 B
examples/demo/dist/chunks/prerender.********.js 293 B 0 B
examples/demo/dist/class-fields/index.html 703 B 0 B
examples/demo/dist/compat/index.html 1.59 kB 0 B
examples/demo/dist/env/index.html 778 B 0 B
examples/demo/dist/error/index.html 708 B 0 B
examples/demo/dist/files/index.html 740 B 0 B
examples/demo/dist/index.********.js 7.65 kB 0 B
examples/demo/dist/index.html 770 B 0 B
examples/demo/dist/json/index.html 710 B 0 B
examples/demo/dist/lazy-and-late/index.html 713 B 0 B
examples/demo/dist/meta-tags/index.html 778 B 0 B

compressed-size-action

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

What a great catch! Can only imagine how long it took to debug this 👍

@marvinhagemeister marvinhagemeister merged commit 7f96b82 into main May 27, 2021
@marvinhagemeister marvinhagemeister deleted the fix/polkompress branch May 27, 2021 08:19
@github-actions github-actions bot mentioned this pull request May 27, 2021
Copy link

@maraisr maraisr left a comment

Choose a reason for hiding this comment

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

🚀👽 to the moon

@developit
Copy link
Member

Wow, awesome debugging here.

lukeed added a commit to developit/polka that referenced this pull request Mar 7, 2024
lukeed added a commit to lukeed/polka that referenced this pull request Mar 7, 2024
* Add @polka/compression package

* skip brotli tests on unsupported node versions

* allow Node>=6

* add stream piping test

* kick CI

* debug: drop action cache

* debug: reattach cache step

* fix: always call `writeHead` step

- see preactjs/wmr#648

* chore: backport fixes;

- now aligns w/ current wmr (and vite) impls

* feat: add dual esm/cjs typescript definitions

* chore: test types

---------

Co-authored-by: Luke Edwards <luke.edwards05@gmail.com>
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.

4 participants