Connection pooling

[inikolaev]

I've noticed that you use requests library in order to make HTTP requests, but requests doesn't use connection pooling by default. In order to use connection pooling one has to create a session via requests.Session() (see here).

Is there any reason why you decided not to use connection pooling?

[hollabaq86]

👋 @inikolaev it looks like this was added to the SDKs 7 years ago for major version 3.0.0, and I don't think there's anyone on our team from that time 🙃

But looking at the commit, it looks like we were simplifying the ways we were making https connection as part of moving to drop python 2.5 and support python 3.3+.

There's definitely benefits to pooling connections, but we also need to make sure we're balancing those benefits without taxing resources. I can take this to our infra teams for their thoughts.

[inikolaev]

@hollabaq86 Sounds good to me! :)

Also looking at the code it looks like it's possible to supply own http_strategy which would handle that, but then some code would have to be duplicated, which might cause incompatibilities during future upgrades.

This could be improved, I think, by moving the default HTTP strategy out of Http into its own class, then subclasses could override it more easily.

[hollabaq86]

Quick update, here. With 4.17.1, we did implement sessions via request.Session() as we discovered an issue around the requests library normalizing URL paths in places we did not expect.

I do think that this conversation does warrant us to take a deeper look at providing the opportunity to supply a custom http_strategy, but that's a consideration for future major versions (and I'd imagine something we'd want to review with async support).

I'm going to go ahead and close this issue for now, but I've noted this internally for future major version updates.