Repository navigation
Make GenerateQuery's query message thread-local - #254
Open
piyush15102003 wants to merge 1 commit into
Open
piyush15102003 wants to merge 1 commit into
piyush15102003 wants to merge 1 commit into
Conversation
GenerateQuery kept the QueryMessage being built in a single static field, so threads building queries at the same time reset and wrote into each other's message. One thread's query could come back with WHERE clauses from several other threads. Store the message in a ThreadLocal, the same way the path field already is, and add a test that builds queries from 8 threads at once. Fixes intuit#223
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Make GenerateQuery's query message thread-local
Fixes #223
Problem
GenerateQuerystores theQueryMessageit is building in a singleprivate staticfield.select()resets that field and then writes into it, so two threads building queries at the same time overwrite each other's message. I reproduced this with 8 threads: on the very first iteration, one thread's query came back aswhich includes the WHERE clauses of 7 different threads. Anyone using the SDK in a multi-threaded server (a web app, or a worker pool syncing several companies) can send corrupted queries this way.
Fix
The message is now held in a
ThreadLocal<QueryMessage>, the same approach this class already uses forpath. The publicgetMessage()/setMessage()/resetQueryMessage()methods keep their signatures, so existing callers are unaffected.Testing
GenerateQueryConcurrencyTest: 8 threads start together on aCyclicBarrier, and each builds 500 queries with thread-specific values and checks the generated string.com.intuit.ipp.querypass, and I ran them 3 times to check for flakiness.Command:
mvn -pl ipp-v3-java-devkit -am test -Dtest="com.intuit.ipp.query.*Test" -Dsurefire.failIfNoSpecifiedTests=false