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

Prevent deadlock on thread synchronization via CountDownLatch #1538

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

Conversation

ljackiewicz
Copy link

This PR resolves reported issue JENKINS-72792 by preventing deadlock on thread synchronization via CountDownLatch.

The changes were tested on an instance running various types of pipelines. No negative impact was noticed.

In case a deadlock occurs while executing a withMaven step (case described in the reported issue), a one-minute timeout is visible.

@ljackiewicz ljackiewicz requested a review from a team as a code owner April 25, 2024 08:58
@ljackiewicz ljackiewicz force-pushed the ljackiewicz/prevent-deadlock-on-thead-sync branch from dd56edd to 8d819ba Compare April 25, 2024 09:21
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not smell right. IIRC a sh step will normally “finish” almost instantly because it is really just forking a process and returning, so that should not be affected by a 1m timeout, but “non-durable” steps like checkout or junit etc. might legitimately run for longer.

It is hard to say from Jira what the real issue is because the line numbers do not seem to match the current version of the pipeline-maven plugin. I am guessing this is https://github.com/jenkinsci/pipeline-maven-plugin/blob/8cfaff9c021c971d19e5469c553a86d954c05387/pipeline-maven/src/main/java/org/jenkinsci/plugins/pipeline/maven/WithMavenStepExecution2.java#L573 which ought not be hanging.

withMaven does tricky stuff and I do not recommend mixing it with the container step generally; it is more straightforward to just run sh 'mvn …'; junit '**/target/surefire-reports/TEST-*.xml'.

@ljackiewicz
Copy link
Author

Okay, I wasn't entirely aware of the impact on non-durable steps.

Referring to the mentioned issue, when calling withMaven step, in my case it looks like the process hangs randomly when calling the join method on the Proc object in the WithMavenStepExecution2.readFromProcess() method (https://github.com/jenkinsci/pipeline-maven-plugin/blob/8cfaff9c021c971d19e5469c553a86d954c05387/pipeline-maven/src/main/java/org/jenkinsci/plugins/pipeline/maven/WithMavenStepExecution2.java#L685). I'm not sure what exactly this problem is related to, but having looked through the ContainerExecProc implementation, I don't quite understand what the reason for using CountDownLatch was, which in this case, at least indirectly, may be causing the problem by not protecting against possible deadlocks.

We have considered discontinuing the use of withMaven step (although a similar problem also occurs when calling other steps), but the functionality of triggering dependent projects is significant to us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants