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

Update Singleton creation pattern to be thread safe #7

Merged
merged 5 commits into from
Aug 13, 2015

Conversation

TuscanRaider
Copy link
Contributor

I believe I found all of the Singletons that needed tweaking. Let me know if I missed any.

TuscanRaider and others added 3 commits August 11, 2015 17:35
… not thread safe. Update code to use a thread safe pattern. Moved the Wizard table init code to the prime method.
… not thread safe. Update code to use a thread safe pattern. Moved the Wizard table init code to the prime method.

Removed commented code
@ai7
Copy link
Contributor

ai7 commented Aug 13, 2015

Nice! You may want to change the indent size (4) to match the current one used in the file (2).

@Narflex
Copy link
Member

Narflex commented Aug 13, 2015

+1 to fixing indent sizes :)

I do have an issue with how this was done. It's changing the timing of when all of these singletons are getting initialized. Now they are will be initialized when the class is initialized...not when the first call to getInstance() is made. This will likely have unintended consequences and will probably even crash when run. For example, if the class loader decides to initialize MMCHolder before the Sage class loads the property system..it will fail to be able to read the properties.

Can you just do it in a way where the logic is the same but fixes that one highly unlikely race condition mentioned in the article? (about the var getting assigned before the object is fully constructed)

… not thread safe. Update code to use a thread safe pattern. Moved the Wizard table init code to the prime method.

Aligned formating to existing style
@TuscanRaider
Copy link
Contributor Author

I have addressed the indenting. I attempted to match the existing style with the braces too, though that's not consistent :) I suppose you can reject the pull request and I will resubmit.

With regard to the initialization timing, the objects are not created until referenced through the getInstance() method (internal classes are not loaded until referenced). The object timing should be consistent with the previous implementation, but faster :)

Ref: https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom

@Narflex
Copy link
Member

Narflex commented Aug 13, 2015

OK, cool...that satisfies me and is a pretty nifty technique. :) One other change then...can you modify CarnyHelper to instead by CarnyHolder so we have consistent naming with this technique?

… not thread safe. Update code to use a thread safe pattern. Moved the Wizard table init code to the prime method.

Renamed Helper -> Holder
@TuscanRaider
Copy link
Contributor Author

Cool. Ooops, yes I meant to fix that. It looks like the pull request updates itself.

Narflex added a commit that referenced this pull request Aug 13, 2015
Update Singleton creation pattern to be thread safe
@Narflex Narflex merged commit c44a409 into google:master Aug 13, 2015
@Narflex
Copy link
Member

Narflex commented Aug 14, 2015

Don't forget to add yourself to the CONTRIBUTORS and AUTHORS files. :)

Narflex pushed a commit that referenced this pull request Sep 18, 2016
JREkiwi pushed a commit to JREkiwi/sagetv that referenced this pull request Dec 30, 2018
Update Singleton creation pattern to be thread safe
JREkiwi pushed a commit to JREkiwi/sagetv that referenced this pull request Dec 30, 2018
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

3 participants