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

Thread safety #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

RyanMentley
Copy link

This is to improve thread safety for static methods that may be called from multiple threads.
This change does not affect the thread safety of individual HttpRequest instances.

Use ThreadLocal for trustedFactory, as the documentation for the classes used makes no thread-
safety guarantees.
Make TRUSTED_VERIFIER final and initialize lazily using the lazy initialization holder class
idiom used in Effective Java item 71, as the implementation used is thread-safe.
Make connectionFactory volatile. This requires implementers of the ConnectionFactory interface
to ensure that their implementation is thread-safe.

This also changes the static members used that were not final to use the more conventional
camelCase naming rather than ALL_CAPS.

This is to improve thread safety for static methods that may be called from multiple threads.
This change does not affect the thread safety of individual HttpRequest instances.

Use ThreadLocal for trustedFactory, as the documentation for the classes used makes no thread-
safety guarantees.
Make TRUSTED_VERIFIER final and initialize lazily using the lazy initialization holder class
idiom used in Effective Java item 71, as the implementation used is thread-safe.
Make connectionFactory volatile.  This requires implementers of the ConnectionFactory interface
to ensure that their implementation is thread-safe.

This also changes the static members used that were not final to use the more conventional
camelCase naming rather than ALL_CAPS.
@RyanMentley
Copy link
Author

Hey, just wondering if there were any issues with this pull request. I'd be happy to address any issues.

@kevinsawicki
Copy link
Owner

Nope, sorry for the delay, will merge it soon.

Do you think it is feasible to add any tests for this that will prove these changes are needed?

@rferreira
Copy link

hey folks, what are the odds of getting this merged in?

@kevinsawicki
Copy link
Owner

@rferreira are you having threading issues using this library?

@rferreira
Copy link

@kevinsawicki nope, just a small false alarm a few weeks back.

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.

3 participants