awslabs / llrt

LLRT (Low Latency Runtime) is an experimental, lightweight JavaScript runtime designed to address the growing demand for fast and efficient Serverless applications.
Apache License 2.0
7.74k stars 342 forks source link

Support for HTTP v2 #338

Closed ShivamJoker closed 2 months ago

ShivamJoker commented 2 months ago

Hey guys great work you've been doing here. I've started to use LLRT instead of NodeJS in few of my projects.

Is there any plan for supporting HTTP2? It will make request duplexing faster.

### Tasks
richarddavison commented 2 months ago

Hi @ShivamJoker. It's already supported by our HTTP client, just not enabled. How would you like to enable it? Via option to fetch, environment variable or some other mechanism? From a pure performance perspective I don't expect much to win here as connections are already being reused and pooled. Maybe If you do lots of request to the same host in parallel

ShivamJoker commented 2 months ago

Oh I had no idea. Thanks for letting me know. Usage with fetch options would be a better choice here IMO.

In NodeJS we have connection reuse variable which works too.

For example if I wanted to send the following with fetch how would I go about that:

curl --http2 -H "accept: application/dns-json" "https://1.1.1.1/dns-query?name=cloudflare.com" 
--next --http2 -H "accept: application/dns-json" "https://1.1.1.1/dns-query?name=example.com"
richarddavison commented 2 months ago

Oh I had no idea. Thanks for letting me know. Usage with fetch options would be a better choice here IMO.

In NodeJS we have connection reuse variable which works too.

For example if I wanted to send the following with fetch how would I go about that:

curl --http2 -H "accept: application/dns-json" "https://1.1.1.1/dns-query?name=cloudflare.com" 
--next --http2 -H "accept: application/dns-json" "https://1.1.1.1/dns-query?name=example.com"

I think browsers by default tries HTTP2 and somehow defaults back to Http 1.1 if the server doesn't support it. I have no idea how this fallback mechanism works, if it inspects the respons and retries with 1.1 if HTTP 2 fails. I will investigate! I think this would be a good option, that can be explicitly disabled if needed.

LLRT_HTTP_VERSION = "auto" | "1.1" | "2"

Connection is always keepAlive and reused in llrt, do you need an option to disable it as well?

ShivamJoker commented 2 months ago

I just wanted to know if HTTP/2 was enabled, I don't want to disable it as of now.

So if I make multiple fetch request to same URL with different query params, the same connection will be used if the API server supports it?

Also does the AWS SDKs which have been bundled in the LLRT use HTTP/2 as well?

richarddavison commented 2 months ago

I just wanted to know if HTTP/2 was enabled, I don't want to disable it as of now.

No it's not enabled right now. My suggestion was to enable it by default and fall back to http 1.1 if server doesn't accept it.

So if I make multiple fetch request to same URL with different query params, the same connection will be used if the API server supports it?

Correct. As long as the request is being fully read, the connection will be reused. For example if you do a fetch() you would have to read the response as well fetch(...).then(res => res.text()) otherwise it will not be reused.

Also does the AWS SDKs which have been bundled in the LLRT use HTTP/2 as well?

If we add this, it is happening inside of ´fetch´ so there is nothing to configure on the SDK side

ShivamJoker commented 2 months ago

No it's not enabled right now. My suggestion was to enable it by default and fall back to http 1.1 if server doesn't accept it.

That would be a better option. Thanks for clarifying.

nabetti1720 commented 2 months ago

Hi @ShivamJoker and @richarddavison ,

No it's not enabled right now. My suggestion was to enable it by default and fall back to http 1.1 if server doesn't accept it.

I did a little research on how this behavior is achieved in hyper-rustls. It seems that hyper-rustls achieves this with the ALPN Protocol, one of the TLS extensions.

https://github.com/rustls/hyper-rustls/blob/78d214b518e9238da26958bc37c699490332283d/src/connector/builder.rs#L387-L429

As far as I can see in the hyper-rustls test case, it seems to be possible to control alpn_protocol by specifying either enable_http1() or enable_http2() or enable_all_version() in the chain method of the ConnectorBuilder.

https://github.com/awslabs/llrt/blob/0ba28cd990ccf4d8b83ebf3c066b28ccc5cc77b8/llrt_core/src/modules/net/mod.rs#L49-L53

If I am correct in this understanding, the implementation is not too difficult and it is already working on my laptop, but I can't think of many use cases that would require the fine control by LLRT_HTTP_VERSION that has been suggested.

What do you think the following would be sufficient to implement?

richarddavison commented 2 months ago

Yes. Can create it in the request. We just need to handle if hyper returns an error that http2 is not supported and retry with HTTP 1.1. PR welcome @nabetti1720 🙂

nabetti1720 commented 2 months ago

Hi @richarddavison , Before I tackle this assignment, let me be sure.

We just need to handle if hyper returns an error that http2 is not supported and retry with HTTP 1.1.

Is this case assuming that the server Hyper connects to only supports http/1.1? In this case, the ALPN Protocol can automatically select http/1.1 in the TLS negotiation without error. If you have a case in mind, please point it out to us so we can consider it. :)

richarddavison commented 2 months ago

Hi @richarddavison , Before I tackle this assignment, let me be sure.

We just need to handle if hyper returns an error that http2 is not supported and retry with HTTP 1.1.

Is this case assuming that the server Hyper connects to only supports http/1.1? In this case, the ALPN Protocol can automatically select http/1.1 in the TLS negotiation without error. If you have a case in mind, please point it out to us so we can consider it. :)

Even better, then it's just a one-liner right? I tired by explicitly adding http2 in request builder which returned an error.

nabetti1720 commented 2 months ago

Even better, then it's just a one-liner right? I tired by explicitly adding http2 in request builder which returned an error.

Sorry, I'm not sure what to do. I thought the following code would accomplish this, but maybe I am thinking too easily.

pub static HTTP_CLIENT: Lazy<Client<HttpsConnector<HttpConnector>, Full<Bytes>>> =
    Lazy::new(|| {
        let pool_idle_timeout: u64 = get_pool_idle_timeout();

        let https = match env::var(environment::ENV_LLRT_HTTP_VERSION).ok().as_deref() {
            Some("1.1") => hyper_rustls::HttpsConnectorBuilder::new()
                .with_tls_config(TLS_CONFIG.clone())
                .https_or_http()
                .enable_http1()                // alpn_protocols is set to vec![b"http/1.1".to_vec()]
                .build(),
            _ => hyper_rustls::HttpsConnectorBuilder::new()
                .with_tls_config(TLS_CONFIG.clone())
                .https_or_http()
                .enable_all_versions()         // alpn_protocols is set to vec![b"h2".to_vec(), b"http/1.1".to_vec()]
                .build(),
        };

        Client::builder(TokioExecutor::new())
            .pool_idle_timeout(Duration::from_secs(pool_idle_timeout))
            .pool_timer(TokioTimer::new())
            .build(https)
    });
richarddavison commented 2 months ago

Even better, then it's just a one-liner right? I tired by explicitly adding http2 in request builder which returned an error.

Sorry, I'm not sure what to do. I thought the following code would accomplish this, but maybe I am thinking too easily.

pub static HTTP_CLIENT: Lazy<Client<HttpsConnector<HttpConnector>, Full<Bytes>>> =
    Lazy::new(|| {
        let pool_idle_timeout: u64 = get_pool_idle_timeout();

        let https = match env::var(environment::ENV_LLRT_HTTP_VERSION).ok().as_deref() {
            Some("1.1") => hyper_rustls::HttpsConnectorBuilder::new()
                .with_tls_config(TLS_CONFIG.clone())
                .https_or_http()
                .enable_http1()                // alpn_protocols is set to vec![b"http/1.1".to_vec()]
                .build(),
            _ => hyper_rustls::HttpsConnectorBuilder::new()
                .with_tls_config(TLS_CONFIG.clone())
                .https_or_http()
                .enable_all_versions()         // alpn_protocols is set to vec![b"h2".to_vec(), b"http/1.1".to_vec()]
                .build(),
        };

        Client::builder(TokioExecutor::new())
            .pool_idle_timeout(Duration::from_secs(pool_idle_timeout))
            .pool_timer(TokioTimer::new())
            .build(https)
    });

Yes, that's it! Except that we don't have to do ok() as we can match on Result instead.

nabetti1720 commented 2 months ago

Yes, that's it! Except that we don't have to do ok() as we can match on Result instead.

Like this?

        let https = match env::var(environment::ENV_LLRT_HTTP_VERSION).as_deref() {
            Ok("1.1") => hyper_rustls::HttpsConnectorBuilder::new()
nabetti1720 commented 2 months ago

I am about to submit a PR, but please forgive me for not being able to write test code. We have accessed a real site that returns the adopted protocol in the response and have confirmed that it works as expected for the following patterns.

richarddavison commented 2 months ago

Landed in https://github.com/awslabs/llrt/pull/357