Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

Requests do not share HTTP(S) connection pool #132

Closed akx closed 5 years ago

akx commented 5 years ago

Hey there,

It looks like msrest (as used by the Azure SDK for Python) does not automatically share a requests context between requests.

I see traces of some code that would allow passing in a requests Session (e.g. here and maybe here too?), but I have no idea how to use those... I'm just creating a ComputeManagementClient :)

The reason why this came up is that I profiled a script that basically just does cmc.virtual_machines.list() about 7 times and it spends half of what it does (29.6 + 17.3 = 46.9%) just connecting to places it could have already have a connection to 😞

Function Calls Own Time %
_ssl._SSLSocket.read 20 2156 39.3%
_ssl._SSLSocket.do_handshake 11 1626 29.6%
_socket.socket.connect 11 949 17.3%
_ssl._SSLContext.load_verify_locations 11 84 1.5%

So... how do I get my ComputeManagementClient to keep that requests session, or better yet, how do I get all of my Client objects to share one session (if that's safe)?

akx commented 5 years ago

Ah, apparently

client = ComputeManagementClient(...)
client.config.keep_alive = True

makes the client instance not close the underlying session. Whether or that's safe, I have no idea...

lmazuel commented 5 years ago

Hi @akx Currently, you have two ways to achieve this:

It was not activated in early SDK, and I didn't activated it by default with current SDK, because that's a breaking change. This code will leak socket connection for example:

for el in myloop():
    client = ComputeClientManagement(**parameters)
    client.virtual_machines.list(**el)

Next major version of client will introduce async, and I decided to make the pool enabled by default with a disclaimer in the ChangeLog.

Hope this clarify :). Thanks!

https://github.com/Azure/msrest-for-python#2018-04-18-version-0428

lmazuel commented 5 years ago

Was commited part of Autorest for Python 4.0.63 as the default behavior: enabled pool per client. This will be shipped at the same time as async support in the next couples of month. Being, if the SDK supports async, it means the connection pool is activated by default.