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

Optionally Pass in the ConnectionContext when creating the TLS Manager #228

Closed
jfischoff opened this issue Sep 15, 2016 · 6 comments
Closed

Comments

@jfischoff
Copy link

For profiling purposes I need to create many (~1000) separate Managers. This is fine for HTTP, but if I need TLS I run into memory and performance issues due to the slow and expensive creation of the ConnectionContext.

I would like to be able to create a single certificate store and share it between Managers.

One way to enable this would be to modify the current mkManagerSettings

mkManagerSettings :: TLSSettings -> Maybe SockSettings -> Maybe ConnectionContext -> ManagerSettings

Alternatively a new function for creation could be added

mkManagerSettingsWithContext :: TLSSettings -> Maybe SockSettings -> Maybe ConnectionContext -> ManagerSettings
@jfischoff jfischoff changed the title Optionally Pass in the ConnectionContext when creating the TL Optionally Pass in the ConnectionContext when creating the TLS Manager Sep 15, 2016
@snoyberg
Copy link
Owner

Check out #227, which has a very similar flavor to this. That PR is blocked on some understanding around laziness of creation, but simply providing a WithContext ability could be done immediately if this is urgent.

@jfischoff
Copy link
Author

I looked closer at the code and see the new mkManagerSettingsContext (https://github.com/snoyberg/http-client/pull/227/files#diff-289ca8a43a0c204643df4a048f8d4ddeR44) which is entirely sufficient for my purposes.

I'm not currently blocked by this FWIW, but will either have to fork http-client temporarily or will become blocked in the next few weeks.

@snoyberg
Copy link
Owner

In that case, I may as well expose mkManagerSettingsContext on master and cut a release. We can deal with the rest of #227 after that.

snoyberg added a commit that referenced this issue Sep 15, 2016
@snoyberg
Copy link
Owner

I just pushed a commit to master with this, want to check it out before I release?

@jfischoff
Copy link
Author

LGTM
On Thu, Sep 15, 2016 at 11:53 AM Michael Snoyman [email protected]
wrote:

I just pushed a commit to master with this, want to check it out before I
release?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#228 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKSO2mef4dfbSFHM_XUaaoC03P0z7f1ks5qqWnsgaJpZM4J96ij
.

@snoyberg
Copy link
Owner

Awesome, released. I'll rebase the other branch on top of this.

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