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

fetch() promise doesn't resolve #26490

Open
Cre3per opened this issue Oct 23, 2024 · 7 comments · May be fixed by #27531
Open

fetch() promise doesn't resolve #26490

Cre3per opened this issue Oct 23, 2024 · 7 comments · May be fixed by #27531
Labels
bug Something isn't working correctly tls Issues related to TLS implementation

Comments

@Cre3per
Copy link
Contributor

Cre3per commented Oct 23, 2024

Version: Deno 2.0.2

Run the following fetch() request

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",
      },
    });

The request won't finish.

Node 22.10.0 finishes the request with an error

Uncaught TypeError: fetch failed
    at node:internal/deps/undici/undici:13392:13
    at async REPL6:1:33 {
  [cause]: HeadersOverflowError: Headers Overflow Error
      at Parser.trackHeader (node:internal/deps/undici/undici:6006:37)
      at Parser.onHeaderValue (node:internal/deps/undici/undici:6001:14)
      at wasm_on_header_value (node:internal/deps/undici/undici:5796:34)
      at wasm://wasm/0002f80e:wasm-function[42]:0x850a
      at wasm://wasm/0002f80e:wasm-function[20]:0x7c38
      at Parser.execute (node:internal/deps/undici/undici:5922:26)
      at Parser.readMore (node:internal/deps/undici/undici:5901:16)
      at TLSSocket.<anonymous> (node:internal/deps/undici/undici:6229:18)
      at TLSSocket.emit (node:events:518:28)
      at TLSSocket.emit (node:domain:552:15) {
    code: 'UND_ERR_HEADERS_OVERFLOW'
  }
}

When running node with node --max-http-header-size 5000000, the promises resolves.

@Cre3per
Copy link
Contributor Author

Cre3per commented Oct 23, 2024

The steps to reproduce depend on an external service (yahoo). It'd be great if someone could capture the response to replay in locally.
I don't know how to do that and unfortunately don't have the time to work on this issue right away. If someone can reproduce this locally, I'd like to work on the issue if the fix isn't urgent.

When using a hyper_tls::client::HttpsConnector instead of proxy::ProxyConnector in ext/fetch/lib.rs create_http_client(), the promise resolves successfully.

@marvinhagemeister marvinhagemeister added bug Something isn't working correctly ext/fetch related to the ext/fetch labels Oct 23, 2024
@lucacasonato
Copy link
Member

Your comment about the HttpsConnector makes it seem like this is TLS related. Strange, but I guess it is possible.

@Cre3per
Copy link
Contributor Author

Cre3per commented Oct 25, 2024

@lucacasonato good call! I tested a bit more, and replacing the hyper_rustls::HttpsConnector with a hyper_tls::HttpsConnector solves the issue. This might be an external bug, or a misconfiguration on Deno's side.

@lucacasonato
Copy link
Member

@littledivy it looks like our tls handling has a bug?

@lucacasonato lucacasonato added tls Issues related to TLS implementation and removed ext/fetch related to the ext/fetch labels Oct 25, 2024
@Cre3per
Copy link
Contributor Author

Cre3per commented Dec 29, 2024

quick status updated

I isolated some of the code for easier debugging. The following function correctly panics in hyper-rustls (hijacked an example with --no-default-features --features="http1,http2,tls12,ring,rustls-native-certs,native-tokio,webpki-tokio"), but blocks in deno (e.g. when called from spawn_subcommand(), which is an arbitrary choice).

As this function doesn't have any explicit dependencies, I suspect there's some global configuration going on in deno.
I'd appreciate a nudge in the right direction at this point. No full solutions please, as this is a good learning opportunity for me.

fn break_me_harder() {
  let _ = rustls::crypto::ring::default_provider().install_default();
  let mut http_connector = HttpConnector::new_with_resolver(GaiResolver::new());
  http_connector.enforce_http(false);

  let root_store = rustls::RootCertStore::from_iter(
    webpki_roots::TLS_SERVER_ROOTS.iter().cloned(),
  );
  let tls = rustls::ClientConfig::builder()
    // .with_native_roots()
    // .unwrap()
    .with_root_certificates(root_store)
    .with_no_client_auth();

  let mut connector_hyper = hyper_rustls::HttpsConnectorBuilder::new()
    .with_tls_config(tls)
    .https_or_http()
    .enable_http1()
    .enable_http2()
    .wrap_connector(http_connector.clone());

  let client =
    hyper_util::client::legacy::Client::builder(TokioExecutor::new())
      .build(connector_hyper);
  let _ = std::thread::spawn(move || {
        let mut request: http::Request<http_body_util::Empty<bytes::Bytes>> = http::request::Request::default();
        // blocks in deno, causes error in hyper-rustls
        *request.uri_mut() = Uri::from_str("https://finance.yahoo.com/markets/currencies/").unwrap();
        // passes
        // *request.uri_mut() = Uri::from_str("https://www.google.com").unwrap();
        request.headers_mut().append("User-Agent", HeaderValue::from_static("Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/123.0.0.0 Safari/537.36"));

          let rt = tokio::runtime::Runtime::new().unwrap();
          dbg!(">block");
          // this unwrap should fail because hyper notices an error
          // blocks in deno
          let r = rt.block_on(client.request(request)).unwrap();
          dbg!("<block");
          r
      })
      .join()
      .unwrap();
  panic!("request finished");
}

output in hyper-rustls (expected)

[examples/client.rs:56:9] ">block" = ">block"
thread '<unnamed>' panicked at examples/client.rs:59:54:
called `Result::unwrap()` on an `Err` value: hyper_util::client::legacy::Error(SendRequest, hyper::Error(Http2, Error { kind: Reset(StreamId(1), PROTOCOL_ERROR, Library) }))

output in deno (bad)

[cli/main.rs:130:11] ">block" = ">block"

@lucacasonato
Copy link
Member

We should be using https://github.com/denoland/rustls-tokio-stream, not hyper-rustls - probably the bug is in https://github.com/denoland/rustls-tokio-stream

Cre3per pushed a commit to Cre3per/deno that referenced this issue Jan 2, 2025
to get hyperium/h2#792

which fixes promises not being resolved when the server sends large
headers
denoland#26490
@Cre3per
Copy link
Contributor Author

Cre3per commented Jan 2, 2025

@lucacasonato Turned out not to be a TLS issue, and unrelated to rustls-tokio-stream, so all good on that! See linked PR for details.

Found the bug by reducing deno to the aforementioned function break_me_harder() and reducing Cargo.toml to only the required dependencies for that function. Diffed with a project that had matching source code and Cargo.toml, but passed. Then noticed the old h2 version in deno's Cargo.lock.

Cre3per added a commit to Cre3per/deno that referenced this issue Jan 3, 2025
to get hyperium/h2#792

which fixes promises not being resolved when the server sends large
headers
denoland#26490
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly tls Issues related to TLS implementation
Projects
None yet
3 participants