Global connection context

[snoyberg]

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.

[vincenthz]

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.

[snoyberg]

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.

[vincenthz]

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.

[Blaisorblade]

IIUC this hunk is just expanding >>= to do-notation (fine) and inlining/copy-pasting tlsManagerSettings—why the inlining? If there's a tricky good reason, a comment would be useful, lest somebody tries removing the duplication.

[snoyberg]

There is a tricky reason: my lack of clarity. I'm not sure if all of the changes I applied here should go through. I was originally planning on just modifying globalManager and not implementing the globalContext, and then I took it a step further and defaulted to everyone sharing a globalContext. Once we're sure that we can use globalContext, I'd revert this hunk. Otherwise, I'd get rid of globalContext and inline the init call to here.

[Blaisorblade]

Ah yes I was wondering. I compulsively use separate commits for such separate steps (so the choice reduces to cherry-picking), but I totally realize that's usually overkill 😅

[snoyberg]

@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?

[ocheron]

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