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

Remove Guice + use Singleton to fix initilization potential deadlock #682

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

Dohbedoh
Copy link

Fixes #635 . Second attempt...
Removed Guice.
Using a Singleton with a simple constructor.
Collectors get register with the Jenkins initialization milestones.
Job related collectors later on when jobs are loaded.

Note that the AsyncPeriodicWork would kick off later on after JOB_CONFIG_ADAPTED https://github.com/jenkinsci/jenkins/blob/jenkins-2.468/core/src/main/java/hudson/model/PeriodicWork.java#L104.

Changes proposed

  • Prevent deadlock on initialization of DefaultPrometheusMetrics
  • Remove Guice injection

Checklist

  • Includes tests covering the new functionality?
  • Ready for review
  • Follows CONTRIBUTING rules

Notify

@Waschndolos

@Waschndolos
Copy link

Thank you for the PR! I'll test it in the next couple of days and come back to you

@Waschndolos
Copy link

The code looks good. I've installed a version of this PR in our Test instance. No issues so far (but didn't have any in the previous attempt, since I'm using a dockered controller). I'll check the instance further and do another test on a non dockered environment. If that's ok as well we might give it a new try :)

@Dohbedoh
Copy link
Author

Sounds good. Thanks @Waschndolos !

@Waschndolos
Copy link

@Dohbedoh. Sorry for the delay, my computer died, had to reinstall 👍 I've installed Jenkins on my unix based system without docker and it doesn't show any error, as well as prometheus endpoint is still working. So I think we're good to go. I'll double check the logs in the companies jenkins tomorrow and if they're ok I'll merge it. Thanks again for the fix!

@Waschndolos Waschndolos merged commit e1c932a into jenkinsci:master Jul 24, 2024
28 checks passed
@Waschndolos
Copy link

Released with 778.ve1c932a_ff24f

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

Successfully merging this pull request may close these issues.

Possible deadlock on startup
2 participants