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(ext/fetch): update h2 to fix sending a PROTOCOL_ERROR instead of REFUSED_STREAM when receiving oversized headers #27531

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Cre3per
Copy link
Contributor

@Cre3per Cre3per commented Jan 2, 2025

Fixes #26490

Related PR in h2 hyperium/h2#792
Related release of h2 https://github.com/hyperium/h2/releases/tag/v0.4.6

No test case, as the changes don't affect deno's code directly and I haven't fully grasped why the promise never resolved. The following script can be used to test manually

// blocks with h2<0.4.6. fails with h2>=0.4.6.
await fetch("https://finance.yahoo.com/currencies/", {
      headers: {
        "User-Agent":
          "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36",
      },
    });

new output

error: Uncaught (in promise) TypeError: error sending request from -snip- for https://finance.yahoo.com/markets/currencies/ (188.125.94.206:443): client error (SendRequest): http2 error: stream error detected: unspecific protocol error detected
     await fetch("https://finance.yahoo.com/markets/currencies/", {
     ^
    at mainFetch (ext:deno_fetch/26_fetch.js:202:11)
    at eventLoopTick (ext:core/01_core.js:175:7)
    at async fetch (ext:deno_fetch/26_fetch.js:454:11)
    at async request.ts:1:6
Caused by: Error: stream error detected: unspecific protocol error detected
    at mainFetch (ext:deno_fetch/26_fetch.js:202:43)
    at eventLoopTick (ext:core/01_core.js:175:7)
    at async fetch (ext:deno_fetch/26_fetch.js:454:11)
    at async request.ts:1:6

This error message is not very helpful, see #26490 for node's output, which explains what went wrong.

  • Do we want to open a new issue to try to improve the error message? (Reported error is hyper_util::client::legacy::Error(SendRequest, hyper::Error(Http2, Error { kind: Reset(StreamId(1), PROTOCOL_ERROR, Library) })), might be non-trivial)
  • Does deno have a flag akin to node's --max-http-header-size which would allow the request to finish successfully? Related Max Header Size #21047 @littledivy

@CLAassistant
Copy link

CLAassistant commented Jan 2, 2025

CLA assistant check
All committers have signed the CLA.

to get hyperium/h2#792

which fixes promises not being resolved when the server sends large
headers
denoland#26490
@Cre3per Cre3per force-pushed the 26490-promise-does-not-resolve branch from 6326bc8 to f3bf9cc Compare January 3, 2025 09:21
@Cre3per
Copy link
Contributor Author

Cre3per commented Jan 3, 2025

force-pushed to correct mail address

@Cre3per
Copy link
Contributor Author

Cre3per commented Jan 3, 2025

check failure looks like a filesystem timing issue unrelated to this PR. I couldn't find other pipelines with this failure though.

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.

fetch() promise doesn't resolve
2 participants