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 CommandLine class initialization deadlock. #21608

Closed

Conversation

benjaminp
Copy link
Collaborator

#21566 (comment) shows a deadlock when initializing static variable CommandLine.EMPTY and another thread is initializing CommandLine's subclass AbstractCommandLine because CommandLine.EMPTY is an instance of AbstractCommandLine.

Fixes #21566.

bazelbuild#21566 (comment) shows a deadlock when initializing static variable `CommandLine.EMPTY` and another thread is initializing `CommandLine`'s subclass `AbstractCommandLine` because `CommandLine.EMPTY` is an instance of `AbstractCommandLine`.

Fixes bazelbuild#21566.
@benjaminp benjaminp requested a review from a team as a code owner March 7, 2024 16:05
@benjaminp benjaminp requested review from aranguyen and removed request for a team March 7, 2024 16:05
@github-actions github-actions bot added team-Performance Issues for Performance teams team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Mar 7, 2024
@fmeum fmeum requested review from meteorcloudy and removed request for aranguyen March 7, 2024 16:08
@meisterT meisterT requested a review from justinhorvitz March 7, 2024 16:12
@meteorcloudy
Copy link
Member

@benjaminp Thanks so much for the fix!

/cc @justinhorvitz Looks like the bug was introduced in b82dcdc

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Mar 7, 2024
@copybara-service copybara-service bot closed this in ae71ce9 Mar 8, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 8, 2024
@justinhorvitz
Copy link
Contributor

@benjaminp thank you so much for the diagnosis and fix. Unfortunately we've already released the culprit change inside google yesterday and are seeing impact, but this PR allowed us to figure out the user complaints of stuck builds and start working on getting the fix released.

@benjaminp benjaminp deleted the benjamin-clinit-deadlock branch March 8, 2024 00:31
fweikert pushed a commit that referenced this pull request Mar 8, 2024
#21566 (comment) shows a deadlock when initializing static variable `CommandLine.EMPTY` and another thread is initializing `CommandLine`'s subclass `AbstractCommandLine` because `CommandLine.EMPTY` is an instance of `AbstractCommandLine`.

Fixes #21566.

Closes #21608.

PiperOrigin-RevId: 613736999
Change-Id: Iadb738b0a89f34cee9a03bc92b385fa7ab58c98f
@justinhorvitz
Copy link
Contributor

As a followup, I submitted a bug report to Intellij because the code inspections did not catch this: https://youtrack.jetbrains.com/issue/IDEA-348744/Static-initializer-references-subclass-inspection-doesnt-report-private-class. The irony is that initially I did get such a warning, but then rearranged the code to make the warning go away.

@benjaminp
Copy link
Collaborator Author

I've run into this type of issue a few times. It's surely one of darkest corners of the JVM especially given the superficial innocuousness of the code that unlocks deadlock gremlins in production. At least maybe this episode will yield better IDE and errorprone checks! IMO, hotspot should also have runtime deadlock detector for this issue. Static analysis cannot detect every case of the deadlock, and a "nice" JVM crash usually beats a hung process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Performance Issues for Performance teams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downstream CI is failing with hanging jobs at Bazel@HEAD
3 participants