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

Fix the deadlock in RunningJobs #333

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
2 participants
@Jimilian
Copy link
Contributor

commented Oct 25, 2017

One more deadlock... :)

threadDump.txt

@@ -769,7 +769,7 @@ public ListBoxModel doFillVerdictCategoryItems() {
*
* @return the store of running jobs.
*/
/*package*/ synchronized RunningJobs getRunningJobs() {
/*package*/ RunningJobs getRunningJobs() {
if (runningJobs == null) {
runningJobs = new RunningJobs();

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 26, 2017

Member

Removing the synchronized here means there is risk of dirty write.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Nov 10, 2017

Author Contributor

synchronized was not needed, runningJobs was supposed to be initialised during class creation, but it was not final. So, I made it final and removed the check.

Did not touch this line at the end.

@@ -2205,7 +2205,7 @@ public String getHelpFile() {
* @param parameters the parameters for the new build, used to find it later.
* @param projectName the name of the current project for better logging.
*/
public synchronized void scheduled(ChangeBasedEvent event, ParametersAction parameters, String projectName) {
public void scheduled(ChangeBasedEvent event, ParametersAction parameters, String projectName) {

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 26, 2017

Member

Removing the synchronized means risk of ConcurrentModificationException below when iterating over runningJobs

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 26, 2017

Member

Well maybe not ConcurrentModificationException but there is no guard against the map being manipulated while iterating it iiuc.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Oct 26, 2017

Author Contributor

I will take a look, this code is in our production for 3 weeks already and everything was fine (at least we did not face deadlocks or CME). But I agree that some guarding can be missed.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Nov 10, 2017

Author Contributor

You were right, we observed non-deterministic behavior during big load (when we start to receive tons of events from here and there). I submitted a new patch, but (!) I think it would be better to wait at least 1 week before merging it :)

This comment has been minimized.

Copy link
@Jimilian

Jimilian Nov 19, 2017

Author Contributor

@rsandell So far looks good.

@Jimilian Jimilian force-pushed the Jimilian:fix_deadlock_in_gerrit_trigger branch 2 times, most recently from 574a18e to 637e290 Nov 10, 2017

Fix the deadlock in RunningJobs
It's not safe to clean the events from queue under the lock, because
this operation needs access to Queue and access to ToGerritListener
class.

Also this commit removes unused variables from method.

@Jimilian Jimilian force-pushed the Jimilian:fix_deadlock_in_gerrit_trigger branch from 637e290 to 7455d86 Nov 10, 2017

@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@rsandell can you take a look please?

@rsandell rsandell merged commit df881de into jenkinsci:master Dec 1, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@Jimilian Jimilian deleted the Jimilian:fix_deadlock_in_gerrit_trigger branch Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.