Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race in ThreadSafeSingleton #322

Merged
merged 1 commit into from Nov 10, 2020
Merged

Fix race in ThreadSafeSingleton #322

merged 1 commit into from Nov 10, 2020

Conversation

rda-dev
Copy link
Contributor

@rda-dev rda-dev commented Nov 10, 2020

The self.__storage attribute may be overwritten by the ThreadSafeSingleton.reset() method and thus will return a None from the _provide(...) method.

Additionally, we should place a return statement inside the lock to prevent reading the shared variable. A more efficient solution is 'double-checked locking optimization.'

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 89.906% when pulling f28abb2 on rda-dev:fix-thread-safe-singleton into 4120afd on ets-labs:develop.

@rmk135
Copy link
Member

rmk135 commented Nov 10, 2020

Hey @rda-dev ,

Thanks for the PR. This looks very good. A cosmetic question before merging - do we need storage temp variable? Does it have any special meaning?

If it's not required, can you, please, remove it in favour of ```self.__storage``?

@rmk135 rmk135 self-assigned this Nov 10, 2020
@rmk135 rmk135 self-requested a review Nov 10, 2020
@rmk135 rmk135 assigned rda-dev and unassigned rmk135 Nov 10, 2020
@rda-dev
Copy link
Contributor Author

rda-dev commented Nov 10, 2020

Hi @rmk135 ,

Yes, the first storage local variable assignment captures an attribute value and allows not to use lock if the variable is already initialized. The second storage assignment may be replaced with return self.__storage statement, but I think it will be clearer to leave a single return from the method and reuse the storage variable here. Isn't it?

@rmk135
Copy link
Member

rmk135 commented Nov 10, 2020

@rda-dev I like to avoid temporary variables when possible. Here is what I meant:

        if self.__storage is None:
            with self.__storage_lock:
                if self.__storage is None:
                    self.__storage = __factory_call(self.__instantiator,
                                                    args, kwargs)
        return self.__storage

It will work the same way, right?

@rda-dev
Copy link
Contributor Author

rda-dev commented Nov 10, 2020

No, it's not the same. In your example, when you release the lock, the parallel thread may set the __storage attribute to None (with reset() method) and you will return None instead of an initialized object.

@rmk135
Copy link
Member

rmk135 commented Nov 10, 2020

Heh, that's tricky. I got the difference finally. Didn't consider reset() effect eventually. Thanks for explanation. Merging.

@rmk135 rmk135 merged commit fb0d99c into ets-labs:develop Nov 10, 2020
@rmk135
Copy link
Member

rmk135 commented Nov 10, 2020

I will cythonize the code and make a release in a couple of hours.

@rmk135
Copy link
Member

rmk135 commented Nov 10, 2020

@rda-dev I have released fix in version 4.3.7. See changelog https://python-dependency-injector.ets-labs.org/main/changelog.html.

I have also added you to the list of contributors https://github.com/ets-labs/python-dependency-injector/blob/master/CONTRIBUTORS.rst. It is distributed with every copy of Dependency Injector.

Thank you again for the fix and for explaining how it works.

Best,
Roman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants