Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Removal of scoped threads for physical actions #34

Merged
merged 4 commits into from
Jan 13, 2023
Merged

Conversation

oowekyala
Copy link
Collaborator

Summary

  • Make ReactionCtx::spawn_physical_thread return a vanilla JoinHandle instead of a ScopedJoinHandle<'x>. This is better for usability as the join handle can be stored in a reactor as a state var, and eg joined on shutdown. This allows more programs to be written: currently some tests like AsyncCallback (link below) are not implementable.
  • To enable this, physical actions do not send Event<'x> directly, because these contain a reference to the internal data structure of the scheduler. Instead, they just send the ID of the action that has been triggered, and let the scheduler fetch that reference itself. It's a real cleanup for the internals of the scheduler.

Rationale

The LF-Rust runtime currently forces the user to use [[scoped threads]] if they want to create a thread that can communicate with the schedule. The reason it is so is to be able to capture references to the internals of the scheduler. It's not a good reason because it could actually be avoided, although it's the historical reason.

There is one debatably good thing that this design gives us, it's that those async threads are guaranteed to be joined by the scheduler when it shuts down the program. I think it's nice because it gives strong semantics to the [[shutdown]] procedure, but it's also maybe not the semantics that we want - maybe it's convenient to let those async threads die alone when the scheduler shuts down.

Using scoped threads actually is otherwise very cumbersome. We have to carry around [[lifetime annotations]] everywhere in the scheduler, which is a source of great pain. It also prevents users from storing threads in reactors and joining them whenever they require it (because otherwise reactors would have to carry lifetime annotations themselves - my attempt at allowing that didn't go well). For instance a program like AsyncCallback cannot be implemented. Ultimately it doesn't net the user more flexibility because the scope of the thread is fixed and only allows taking references to scheduler internals, which LF users don't have access to. It's only an implementation artefact that gets exposed to the user through the interface.

@oowekyala oowekyala requested review from cmnrd and jhaye October 28, 2022 12:01
Copy link
Contributor

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

This looks great! I have two questions:

  1. Should this be accompanied by a PR in LF that implements some tests like AsyncCallback?
  2. Does this fix the race condition we observed for assigning the tag?

@oowekyala oowekyala merged commit 7685764 into main Jan 13, 2023
@oowekyala oowekyala deleted the no-scoped-threads2 branch January 13, 2023 12:20
@lhstrh lhstrh changed the title Remove scoped threads for physical actions Removal of scoped threads for physical actions Feb 23, 2023
@lhstrh lhstrh added the enhancement New feature or request label Feb 23, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants