http-client icon indicating copy to clipboard operation
http-client copied to clipboard

Global connection context

Open snoyberg opened this issue 9 years ago • 5 comments

Pinging @sjakobi and @vincenthz. This is intended to speed up manager creation (#214). However, I'm not sure how safe it is to have a global connection context like this. @vincenthz can you weigh in on this approach? Once I get an idea of the semantics of this, I'll clean up this commit and ask for a second review.

snoyberg avatar Sep 13 '16 05:09 snoyberg

yes, this is supposed to be much faster, as the system store would be loaded only once, and shared by all subsequent TLS connections. I haven't checked that it works under a unsafePerformIO context, but I don't see any reason why not. One thing that I'm worried about with this, is whether or not there's going to be any lazy IO pending until the certificate store is used.

vincenthz avatar Sep 13 '16 06:09 vincenthz

As I've written the code, there definitely will be laziness. The question is whether this is a good thing or not. Options:

  1. When calling newManager, force the evaluation of the global context. This ensures that we deterministically perform that action when creating the manager. We'd likely want to apply strictness to getGlobalManager too then. This would ensure that IO is only performed in an explicit IO call.
  2. Leave as-is, and the first time we make an HTTPS connection, the thunk will be forced, making the first connection taking longer than subsequent connections. On the other hand: no overhead at all if we only make insecure connections. Only other concern is if multiple threads are accessing the thunk at the same time, but in practice GHC's runtime should make that situation OK.

snoyberg avatar Sep 13 '16 06:09 snoyberg

I think it would make sense that the whole of the certificate store is strictly loaded once, only after it's been really needed (so lazy from the outside point of view, but strict inside); I think that's the best outcome, as it still allow fast HTTP without needing the cert store. But I haven't checked that the certificate store loading does the right thing here.

vincenthz avatar Sep 13 '16 06:09 vincenthz

@vincenthz Just to confirm, I should hold off on merging this PR (or any version of it) until you've had a chance to check that, right?

snoyberg avatar Sep 15 '16 12:09 snoyberg

From an API standpoint it would be more general to have:

mkManagerSettingsContext
    :: IO NC.ConnectionContext
    -> NC.TLSSettings
    -> Maybe NC.SockSettings
    -> ManagerSettings

ManagerSettings is the receipe used by the manager to build connections. With this API I can supply an IO-based function to decide exactly what to do:

  • allocate a new ConnectionContext for each new Manager
  • reuse a global ConnectionContext until it is too old and must be recycled
  • reuse a global ConnectionContext indefinitely

ocheron avatar Oct 31 '16 13:10 ocheron