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

feat: builder with shared configuration #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Fishrock123
Copy link
Contributor

Sometimes one might want to share the config to the builder api like one can to the connector api itself, allowing some optimization.

Since the config eventually get's Arc-d anyways there can be no disadvantage to this.

@Fishrock123 Fishrock123 force-pushed the builder-shared-config-optimization branch from cbc9fa8 to 94bd6ce Compare March 15, 2024 22:45
src/connector/builder.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Mar 18, 2024

If htis is for reqwest, not sure this still make sense if it wants to just set its own fully-configured ClientConfig? This seems like a pretty niche use case and I'm not sure this is the best way to cover it.

@Fishrock123
Copy link
Contributor Author

It wants to set it's own fully configured ClientConfig but already holds an Arc-shared reference, hence this PR.

I can imagine other clients may like this as well.

Sometimes one might want to share the config to the builder api like one can to the connector api itself, allowing some optimization.

Since the config eventually get's `Arc`-d anyways there can be no disadvantage to this.
@Fishrock123 Fishrock123 force-pushed the builder-shared-config-optimization branch from 94bd6ce to 4a59f1e Compare March 19, 2024 20:21
@Fishrock123
Copy link
Contributor Author

Updated to have pub fn with_tls_config(self, config: impl Into<Arc<ClientConfig>>) ...

@djc
Copy link
Member

djc commented Mar 20, 2024

Why does it hold an Arc-shared reference? I don't like the use of Arc::make_mut() here -- it seems weird to me to want to use the HttpConnectorBuilder for a ClientConfig that is already "shrinkwrapped" in Arc.

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Mar 20, 2024

Why does it hold an Arc-shared reference?

I'll have to spend more time in the codebase to answer that in an exact way - I don't know offhand, if I had to guess from past experience with Surf then connection pooling seems like a likely reason.

I could change this to use Arc::get_mut if you like, which could possibly be a better way of doing things?

Comment on lines +1 to +3
{
"rust-analyzer.cargo.features": "all"
}
Copy link
Member

Choose a reason for hiding this comment

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

Was this part of the diff intentional? I think we'd prefer to see a .vscode addition to .gitignore in a stand-alone commit and this file backed out. There isn't an established precedent for committing IDE settings to the Rustls repos AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unintentional, but it might take me a bit to get to it now I've got some life stuff occuring

Copy link
Member

Choose a reason for hiding this comment

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

No worries, best of luck.

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.

3 participants