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

Enabling HTTP2 crashes at runtime on client #287

Closed
0xAlcibiades opened this issue Sep 12, 2024 · 7 comments
Closed

Enabling HTTP2 crashes at runtime on client #287

0xAlcibiades opened this issue Sep 12, 2024 · 7 comments

Comments

@0xAlcibiades
Copy link

Working on some benchmarks for a new server.

Unable to use the http2 client.

Finished `bench` profile [optimized + debuginfo] target(s) in 54.62s
     Running benches/hello_world_tower_hyper_tls_tcp.rs (target/release/deps/hello_world_tower_hyper_tls_tcp-a580cce80814937a)
Benchmarking hyper_server/serial_latency
Benchmarking hyper_server/serial_latency: Warming up for 5.0000 s
thread 'tokio-runtime-worker' panicked at /Users/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-1.4.1/src/common/time.rs:37:17:
You must supply a timer.
stack backtrace:

at runtime, when I enable:

        let https = HttpsConnectorBuilder::new()
            .with_tls_config(tls_config.client_config)
            .https_or_http()
            .enable_all_versions()
            .build();

        let client: Client<_, Empty<Bytes>> = Client::builder(TokioExecutor::new())
            .timer(TokioTimer::new())
            .pool_timer(TokioTimer::new())
            .build(https);

http1 works just fine.

Whole file:

https://github.com/warlock-labs/hyper-server/blob/ff7b4397fa1a0488dd5534fc46c27d21248bd5d7/benches/hello_world_tower_hyper_tls_tcp.rs

@0xAlcibiades 0xAlcibiades changed the title Enable HTTP2 crashes at runtime Enable HTTP2 crashes at runtime on client Sep 12, 2024
@0xAlcibiades 0xAlcibiades reopened this Sep 12, 2024
@djc djc changed the title Enable HTTP2 crashes at runtime on client Enabling HTTP2 crashes at runtime on client Sep 12, 2024
@djc
Copy link
Member

djc commented Sep 12, 2024

It crashes in hyper, ostensibly from a lack of setting up a timer. I don't think this has anything to do with hyper-rustls.

@0xAlcibiades
Copy link
Author

It's interesting, same thing with reqwest, but the runtime does indeed have a timer available.

@0xAlcibiades
Copy link
Author

It seems to me that there is something going on here around the new hyper 1.x runtime expecting to find a tokio timer, but not seeing it due to something?

fn create_optimized_runtime(thread_count: usize) -> io::Result<Runtime> {
    tokio::runtime::Builder::new_multi_thread()
        .worker_threads(thread_count)
        .max_blocking_threads(thread_count * 2)
        .enable_all()
        .build()
}

It is, however, enabled

tokio = { version = "1.40", features = ["rt-multi-thread", "net", "test-util", "time"] }

and the feature flag present.

@ctz
Copy link
Member

ctz commented Sep 12, 2024

Do you have the http2 feature set on hyper-util? That is what hooks up its timer method to feed hyper's Client::Builder::timer, which sets the timer that is required.

@0xAlcibiades
Copy link
Author

I do, yes, it does seem like there was the same traceback server side on sleep as well as the client.

@0xAlcibiades
Copy link
Author

I wonder if criterion messes with the timer in some unexpected way.

@0xAlcibiades
Copy link
Author

    // Configure the builder
    let mut builder = builder.clone();
    builder
        // HTTP/1 settings
        .http1()
        // Enable half-close for better connection handling
        .half_close(true)
        // Enable keep-alive to reduce overhead for multiple requests
        .keep_alive(true)
        // Increase max buffer size to 256KB for better performance with larger payloads
        .max_buf_size(256 * 1024)
        // Enable immediate flushing of pipelined responses
        .pipeline_flush(true)
        // Preserve original header case for compatibility
        .preserve_header_case(true)
        // Disable automatic title casing of headers to reduce processing overhead
        .title_case_headers(false)
        // HTTP/2 settings
        .http2()
        // Add the timer to the builder
        // This will cause you all sorts of pain otherwise
        // https://github.com/seanmonstar/reqwest/issues/2421
        // https://github.com/rustls/hyper-rustls/issues/287
        .timer(TokioTimer::new())
        // Increase initial stream window size to 2MB for better throughput
        .initial_stream_window_size(Some(2 * 1024 * 1024))
        // Increase initial connection window size to 4MB for improved performance
        .initial_connection_window_size(Some(4 * 1024 * 1024))
        // Enable adaptive window for dynamic flow control
        .adaptive_window(true)
        // Increase max frame size to 32KB for larger data chunks
        .max_frame_size(Some(32 * 1024))
        // Allow up to 2000 concurrent streams for better parallelism
        .max_concurrent_streams(Some(2000))
        // Increase max send buffer size to 2MB for improved write performance
        .max_send_buf_size(2 * 1024 * 1024)
        // Enable CONNECT protocol support for proxying and tunneling
        .enable_connect_protocol()
        // Increase max header list size to 32KB to handle larger headers
        .max_header_list_size(32 * 1024)
        // Set keep-alive interval to 10 seconds for more responsive connection management
        .keep_alive_interval(Some(Duration::from_secs(10)))
        // Set keep-alive timeout to 30 seconds to balance connection reuse and resource conservation
        .keep_alive_timeout(Duration::from_secs(30));

This ended up being a hyper::server::conn::auto::Builder ergonomics issue.

@ctz ctz closed this as not planned Won't fix, can't repro, duplicate, stale Sep 12, 2024
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

No branches or pull requests

3 participants