-
Notifications
You must be signed in to change notification settings - Fork 1
Description
I have never (knowingly) been tripped up by it, but I noticed this while reviewing #137...
gapipy.client.default_config is basically used as a default argument to client initialization and some of its content are (mutable) dicts. Because of that it's possible to accidentally "share" data there -- you might have some surprises if you instantiate multiple clients in the same process with different configs.
To demonstrate...
>>> from gapipy import Client
>>> # Let's make a client and enable connection pooling
>>> c1 = Client(connection_pool_options={"enable": True})
>>> c1.connection_pool_options["enable"]
True
>>> # Ok, let's make another WITHOUT connection pooling
>>> c2 = Client(connection_pool_options={"enable": False})
>>> c2.connection_pool_options["enable"]
False
>>> # Cool, now let's see what that first client is up to, it was True to begin with...
>>> c1.connection_pool_options["enable"]
False
>>> c1.connection_pool_options is c2.connection_pool_options
TrueI suspect the crux of the issue is probably twofold:
- new client instances get direct references to stuff in default_config and some of those things are mutable
Client.__init__explicitly mutates the defaultconnection_pool_optionsdict when some extra connection pool configs have been passed
I bet you could have a similar issue with the global http headers config, except that Client.__init__ doesn't mutate that one you'd have to have a series of events like:
- instantiate a client
- instantiate another client
- mutate one of their
client.global_http_headersand the other client is affected because they both shared the same default value
(Pretty sure I had a hand in both of those connection-pool-options and global-http-headers things 🤦 oops)
I haven't taken a run at writing tests for it yet, but I wonder if the fix is simply:
- update
get_configtocopy.deepcopy(or similar) the value it gets fromdefault_configbefore returning it, and - use
get_config(instead of direct dict access) when getting"connection_pool_options"out ofdefault_config
We could also take other approaches like:
Client.__init__should deepcopy the default dict before yanking stuff out, or- the default dict should be returned from a function instead of existing at at module level, or
- something else entirely 🤷