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: Prevent deadlocks when mixing threads with queue based execution context #3852

Conversation

WojciechMazur
Copy link
Contributor

  • Prevent deadlocks or pre-early exit when mixing threads with queue based execution context
  • NativeThread.registry.aliveThreads now returns iterator instead of array
  • Use thread-safe queue in QueueExecutionContext - there is always 1 consumer, but there can be multiple producers

Can help with resolving #3848 - the scaltest framework snapshot might have been adding new tasks to the queue.

… queue based execution context

NativeThread.registry.aliveThreads now returns iterator instead of array
@WojciechMazur WojciechMazur merged commit 677b1bc into scala-native:main Mar 28, 2024
61 checks passed
@WojciechMazur WojciechMazur deleted the fix/prevent-deadlock-in-mt-with-queue-ec branch March 28, 2024 13:23
@ekrich
Copy link
Member

ekrich commented Mar 28, 2024

@WojciechMazur This is very nice. What is the purpose of naming a class package.state.scala? Not familiar with that convention / choice.

@WojciechMazur
Copy link
Contributor Author

What is the purpose of naming a class package.state.scala? Not familiar with that convention / choice.

The main purpose here was to cleanup the access to the statefull data. We don't want to store any state in runtime directly - this way when calling any of the methods defined in it, we would not need to obtain the runtime object instance. Instead we define them in seperate objects. The problem is that it's defined as package object runtime. Becose of that we cannot restrict access of the state container to be private[runtime]. So we moved them to another file that is defined in package scala.scalanative.runtime.

If the state would not be referenced from outside the file then we could have just used private access modifer, but we want to send notification from other runtime entities - from NativeThread or QueueExecutionContext

The naming might be confusing, after all that might not be allowed in Java, so it's not familiar in the Scala. However, we already do something like that in scala-cli, where we can define tests using MyLogic.test.scala to place the test sources in the same directories as the main sources. There isn't anything blocking us from that, and it seemed to be the most clean solution as the state and the stateless source files are placed next to each other

@ekrich
Copy link
Member

ekrich commented Mar 28, 2024

Thank you for the explanation. I hadn't seen that naming before and the info about package object and access control is good info too. Fine tuned design is an art and I am not really at that level.

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.

None yet

2 participants