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

Shared reqwest client between fetch calls #6792

Merged
merged 1 commit into from
Jul 18, 2020

Conversation

lucacasonato
Copy link
Member

Maybe fixes #6696

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - that was easier than I expected it to be. Nice work.

@ry ry merged commit 071a6e2 into denoland:master Jul 18, 2020
@lucacasonato lucacasonato deleted the fetch_into_state branch July 18, 2020 19:32
@rudoi
Copy link

rudoi commented Jul 18, 2020

Curious, does this complicate #3444 at all? Should the presence of additional arguments signal that we should not use the global client?

@Spoonbender
Copy link
Contributor

@rudoi in connection pools implementations I've seen, the pool is keyed in a way which represents all relevant input properties of the connection. In other words, the additional arguments shouldn't cause Deno to use a non-pooled connection - instead, there should be a pool for each unique permutation of the arguments that is being used. In the DB connection pooling world it is fairly easy: the key is the connection string (after some normalization). In Deno's HTTP client pooling, we can perhaps serialize the input arguments to JSON and use that for keying.

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.

Consequent fetch requests sometime doesn't use DNS cache
4 participants