En-threadsafe json injections for collections #224
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The existing fromBuilder method reused the same builder for multiple `invert` calls, which is not safe under multithreading: one thread might, for example, clear the builder while another thread is adding elements. This patch changes the implementation to get a new builder every time from the CanBuildFrom, and puts that behind the existing fromCollection interface. The existing injections for specific collections should not change behaviour.
|
Thanks for catching this! |
johnynek
added a commit
that referenced
this pull request
Jul 20, 2015
En-threadsafe json injections for collections
|
NP! Thanks for the pull. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi!
Currently, the injections produced by the
JsonInjectionimplicits are not threadsafe, since they reuse the same builder instance. If two different threads call intoinvertat the same time, both threads will clear and then add items to that builder, and the results of this are undefined.This patch switches to using a brand-new builder instance for every call to invert. The resulting injections should be safe to use from multiple threads.
This involves a small change to the API: in particular, it drops the
fromBuildermethod and leans everything oncollectionJson. Seemed mostly harmless, since thefromCollectionmethod implements roughly the same functionality in a safer way -- but it is a breaking change.