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

Comments

Projects
None yet
2 participants
@jfischoff
Copy link

jfischoff commented Sep 15, 2016

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

This comment has been minimized.

Copy link
Owner

snoyberg commented Sep 15, 2016

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

This comment has been minimized.

Copy link
Author

jfischoff commented Sep 15, 2016

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

This comment has been minimized.

Copy link
Owner

snoyberg commented Sep 15, 2016

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

This comment has been minimized.

Copy link
Owner

snoyberg commented Sep 15, 2016

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

@jfischoff

This comment has been minimized.

Copy link
Author

jfischoff commented Sep 15, 2016

LGTM
On Thu, Sep 15, 2016 at 11:53 AM Michael Snoyman notifications@github.com
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

This comment has been minimized.

Copy link
Owner

snoyberg commented Sep 15, 2016

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

@snoyberg snoyberg closed this Sep 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.