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

did:web resolver HTTP client creation is very slow #516

Open
vdods opened this issue Jun 27, 2023 · 3 comments
Open

did:web resolver HTTP client creation is very slow #516

vdods opened this issue Jun 27, 2023 · 3 comments

Comments

@vdods
Copy link
Contributor

vdods commented Jun 27, 2023

In optimizing my verification codepaths, I've noticed that a major bottleneck was the creation of a new reqwest::Client for each did:web DID doc resolution. It's mostly because loading up all the TLS certs is a pretty involved operation. The reqwest crate documentation ( https://docs.rs/reqwest/latest/reqwest/struct.Client.html ) says

The Client holds a connection pool internally, so it is advised that you create one and reuse it.
You do not have to wrap the Client in an Rc or Arc to reuse it, because it already uses an Arc internally.

I fixed this issue in my own fork by creating a reqwest::Client singleton via lazy_static (example below), and then cloning that to make HTTP requests. In my particular verification-heavy codepath, this led to reducing the run time by over 60%!

This lazy_static approach is the easiest and laziest solution. However, if there are other crates that also do similar and have their own lazy_static reqwest::Client singletons, having a redundant one be less than ideal. I don't know a good approach for how to handle that. Maybe there could be an initialization function that is optionally called to set the reqwest::Client singleton to be a clone of the one supplied to that initialization function.

Anyway, because of how severe a bottleneck this is, I'd suggest we address this somehow in the did-web crate.

I put the following in did-web/src/lib.rs:

lazy_static::lazy_static! {
    /// Building a reqwest::Client is *incredibly* slow, so we use a global instance and then clone
    /// it per use, as the documentation indicates.
    pub static ref REQWEST_CLIENT: reqwest::Client = {
        let mut headers = reqwest::header::HeaderMap::new();

        headers.insert(
            "User-Agent",
            reqwest::header::HeaderValue::from_static(USER_AGENT),
        );

        let client = match reqwest::Client::builder().default_headers(headers).build() {
            Ok(c) => c,
            Err(err) => { panic!("Error building HTTP client: {}", err.to_string()) }
        };

        client
    };
}
@sbihel
Copy link
Member

sbihel commented Jun 28, 2023

I agree that the current implementation is not great, I would actually put the client in DIDWeb

@vdods
Copy link
Contributor Author

vdods commented Jun 29, 2023

Ok -- perhaps there can be two methods: (1) an optional initialize method, in case you want to supply your own reqwest::Client ahead of time, and (2) a getter for the reqwest::Client, which creates the thing if it doesn't exist already. That way, we still get the lazy behavior, but it can be made non-redundant if you care to.

If that sounds good, I'll make a PR.

@sbihel
Copy link
Member

sbihel commented Jun 30, 2023

I don't know if this is the right place to add laziness, I think it's cleaner to have a fn new(client: Option<Client>) -> Self which either use the provided client or create a new one. Then depending on the use case or environment the DIDWeb can be constructed lazily, either directly or through DIDMethods (which would require changes)

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

2 participants