The library is not thread-safe

[dylanahsmith]

ActiveResource doesn't seem to be suitable for multi-threaded applications because state is stored on the class that multiple threads may want to modify and use at the same time.

Perhaps this could be solved by storing the connection in a thread-local variable. In this way, a new connection could lazily be created for each thread when the ShopifyResource.connection property is accessed, and no locking would be needed in order to concurrently make requests from different threads.

[dylanahsmith]

It looks like the user, password, site, and timeout properties on ShopifyResource should all just change the state of the connection object (accessed through the connection property to make sure it exists). Currently this is done by setting the _connection variable to None, but it looks like re-using the connection object is safe if its instance variables are set properly.

The protected variables _user, _password, _site and _timeout could still get set on ShopifyResource after changing the connection object. This would allow other threads to use these settings by default before the connection property is accessed the first time, creating the connection object, but afterwards they would need to be explicitly modified by the current thread to modify the already created connection object.

[dylanahsmith]

Currently, it looks like connection objects are being stored on the class, so a separate connection object will be created on all the resources. This should be changed to have the connection object get explicitly set and retrieved from the ShopifyResource class. This would also make it easier to change the site variable and have it affect each resource.

EDIT: This appears to be a regression introduced in v0.2.0 of this library, so I'll fix it and release v0.2.1.
EDIT 2: Fixed.
EDIT 3: It looks like per-resource connections could always be made by setting the resources site, user, password, timeout, or _connection variables. I could see why this is useful at the pyactiveresource level, but I don't think this is needed for classes derived from ShopifyResource.

[dave4jr]

Hey @dylanahsmith Was just curious if you ever had success implementing this and if so, why did you guys decide to not move forward with integrating this commit? I'm working on something similar and was just curious what your thoughts are on this. Thanks!

[dylanahsmith]

Was just curious if you ever had success implementing this

See the commit that closed this issue (c3a344f)

if so, why did you guys decide to not move forward with integrating this commit?

I'm confused, the issue was closed by integrating that commit. The code on master is pretty similar, although has since been updated to support python 3.

[dave4jr]

Yeah I saw that you committed it to a feature branch, but I wasn't sure if you guys actually merged it into a release. Sorry that was my fault for not looking close enough.