Pipeline not getting unlocked upon a successful run - stale cache during race condition #3497
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
While trying to lock/unlock a pipeline, we used to clear the cache and update the db entry within a synchronized block. When an unlock thread exits the synchronized block but hasn't executed a transaction.commit, immediately followed by a read thread ie. pipelineStateFor, the (eh)cache would have been updated with a stale entry from the query-cache (since cacheable was set to true). This cache entry is not cleared thereafter, ie. even after the transaction.commit of unlock happens. This used to lead to a bug wherein some of the pipelines would show up as locked on dashboard even when they were unlocked. Now, we clear off the ehcache entry only after a transaction commit. This means some of the reads could potentially get seemingly stale data even after the unlock thread exits the synchronized block and before the transaction.commit happens, but that should be ok.
jyotisingh
changed the title
jyotisingh
changed the title
bhimsenpadalkar
pushed a commit
to bhimsenpadalkar/gocd
that referenced
this pull request
Jun 14, 2017
* Fixing an issue with pipeline state read/save order While trying to lock/unlock a pipeline, we used to clear the cache and update the db entry within a synchronized block. When an unlock thread exits the synchronized block but hasn't executed a transaction.commit, immediately followed by a read thread ie. pipelineStateFor, the (eh)cache would have been updated with a stale entry from the query-cache (since cacheable was set to true). This cache entry is not cleared thereafter, ie. even after the transaction.commit of unlock happens. This used to lead to a bug wherein some of the pipelines would show up as locked on dashboard even when they were unlocked. Now, we clear off the ehcache entry only after a transaction commit. This means some of the reads could potentially get seemingly stale data even after the unlock thread exits the synchronized block and before the transaction.commit happens, but that should be ok.
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.
The PipelineState maintains the pipeline lock status of a pipeline. A significantly high number of queries are made to the PipelineStateDAO to check the pipeline lock status. To avoid frequent queries to database the DAO maintains a cache of the PipelineState and was cleared upon a lock or unlock, which was done as part of a synchronised block. We came across a scenario where the cache got out of sync with the db, below is the scenario.
When an unlock thread exits the synchronized block but hasn't executed a transaction.commit, immediately followed by a read thread ie. pipelineStateFor, the (eh)cache would have been updated with a stale entry from the query-cache (since cacheable was set to true). This cache entry is not cleared thereafter, ie. even after the transaction.commit of unlock happens. This used to lead to a bug wherein some of the pipelines would show up as locked on dashboard even when they were unlocked.
Now, we clear off the ehcache entry only after a transaction commit. This means some of the reads could potentially get seemingly stale data even after the unlock thread exits the synchronized block and before the transaction.commit happens, but that should be ok.