Skip to content

Fix deadlock in lazy intialization of CoreBTypes #19297

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

Closed

Conversation

WojciechMazur
Copy link
Contributor

@WojciechMazur WojciechMazur commented Dec 19, 2023

Fixes #19293 caused by double synchronisation of lazy val initialisation. When PostProcessor thread tried to access CoreBTypesFromSymbols.jliLambdaMetaFactoryAltMetafactoryHandle in BackendUtils.collectSerializableLambdas it could have one the data race for initialisation of lazy val with the main GenBCode thread, but it could have failed the synchronisation race with frontendSynch leading to deadlock.
To fix this issue we use @threadUnsafe lazy val so the synchronisation is only based on the frontendSynch lock.

Usage of @threadUnsafe lazy vals makes semantics of CoreBTypes initialisation more aligned with Scala 2 backend (threadUnsafe lazy vals is almost identical with Scala2 lazy val)

Alternative solution to this issue is to create a PostProcessor/BackendUtils local jliLambdaMetaFactoryAltMetafactoryHandle instance using mangled Strings with descriptor which would not use any Symbols or Context, but it seems be more error-prone in the long term maintenance.

@WojciechMazur WojciechMazur requested a review from sjrd December 19, 2023 10:54
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

This will fix the deadlock, indeed. However, it can introduce other issues. If two threads try access the same @threadUnsafe lazy val, even if the initialization code is protected, the assignment to the field and the bitmap will not be protected. Although unlikely, this can lead to NPEs if another thread sees the updated bitmap while not seeing the updated actual value. More likely: two threads accessing two different lazy vals of the same object can concurrently try to update the same bitmap, leading to corruption.

Conclusion: @threadUnsafe lazy vals are thread-unsafe no matter how thread-safe their user-written right-hand-side is.

Usage of @threadUnsafe lazy vals makes semantics of CoreBTypes initialisation more aligned with Scala 2 backend (threadUnsafe lazy vals is almost identical with Scala2 lazy val)

That is not true. Scala 2 lazy vals are thread-safe. They just don't use the same mechanism as Scala 3.

@WojciechMazur
Copy link
Contributor Author

Thank you @sjrd! In such case probably we could port the Scala 2 LazyVar used for initialisation of values in CoreBTypes with exception of reinitialization part which would be unused in Scala 3 (we do never reuse CodeGen components for different runs, only for different compilation units within the same run)
https://github.com/scala/scala/blob/c80c0ff0863ce8c1674b249e741a13c064cdd5b0/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala#L1081-L1108

@sjrd
Copy link
Member

sjrd commented Dec 19, 2023

Ah yes, that seems to be what we need.

@WojciechMazur
Copy link
Contributor Author

Replaced with #19298

@WojciechMazur WojciechMazur deleted the fix/parallel-backend-deadlock branch December 19, 2023 13:41
WojciechMazur added a commit that referenced this pull request Dec 20, 2023
…9298)

Replaces #19297 and fixes #19293 

The deadlocks are now fixed by introduction of
`PostProcessorFrontendAccess.Lazy[T]` container for which initialization
is synchronized with the frontend Context, while providing a thread-safe
access lacking in original solution.

It now also audits where the unrestricted access to context was used and
replaces these usages with safer access.
Reverts #19292
odersky pushed a commit to dotty-staging/dotty that referenced this pull request Jan 6, 2024
…ala#19298)

Replaces scala#19297 and fixes scala#19293 

The deadlocks are now fixed by introduction of
`PostProcessorFrontendAccess.Lazy[T]` container for which initialization
is synchronized with the frontend Context, while providing a thread-safe
access lacking in original solution.

It now also audits where the unrestricted access to context was used and
replaces these usages with safer access.
Reverts scala#19292
WojciechMazur added a commit that referenced this pull request Jul 10, 2024
…9298)

Replaces #19297 and fixes #19293 

The deadlocks are now fixed by introduction of
`PostProcessorFrontendAccess.Lazy[T]` container for which initialization
is synchronized with the frontend Context, while providing a thread-safe
access lacking in original solution.

It now also audits where the unrestricted access to context was used and
replaces these usages with safer access.
Reverts #19292
[Cherry-picked 33bdaac][modified]
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.

Parallel backend can deadlock when accessing the synchronizing access to frontend.
2 participants