Skip to content

Conversation

@vtjnash
Copy link
Member

@vtjnash vtjnash commented Aug 19, 2025

Clarify this code does not have access to a TaskDispatcher, and thus is not permitted use of threads, by using a more efficient single-threaded monostate variant, instead of making it appear that this code could potentially be a cause of thread-safety concerns (notably deadlock).

(Written by Claude)

Clarify this code does not have access to a TaskDispatcher, and thus is
not permitted use of threads, by using a more efficient single-threaded
monostate variant, instead of making it appear that this code could
potentially be a cause of thread-safety concerns (notably deadlock).
@vtjnash vtjnash requested a review from lhames August 19, 2025 14:56
Copy link
Contributor

@lhames lhames left a comment

Choose a reason for hiding this comment

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

Hi @vtjnash. Sorry for the delayed review!

Rather than this change, I think we should revert my patch 3b5842c that introduced the async versions of runAllocActions and runDeallocActions: it was a failed experiment. I apologize for not getting around to removing it earlier.

Background: Deallocation actions are allowed to reside in JIT'd memory, but this makes asynchronous deallocation actions unsafe. E.g. frame-info-registration/deregistration is loaded as part of the JIT runtime (so resides in JIT'd memory), and most object files will contain a deallocation sequence like [..., deregister-frame-info, ..., deallocate-memory ]. If deregister-frame-info is asynchronous and tail-call-elimination doesn't kick in (which it won't if we actually make use of the asynchrony) then you end up with a call-stack that looks like:

#0 deallocate-memory
...
#N deregister-frame-info
...

For the JIT runtime object that introduces the deregister-frame-info action, this will lead to us unwinding through the deregister-frame-info action that was just deallocated. (This was observed in practice on an experimental branch).

Asynchronous deallocation actions are also incompatible with a future planned "detach" operation that would allow JIT'd code to keep running after the controller disconnects.

Between those two things it seems better for now to require allocation and deallocation actions to be self-contained, synchronous operations.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 4, 2025

Yep. I'll leave this open until the revert is done, but that sounds better to me.

Aside: that sort of design pattern is why the wait-for-complete scheduler function PR exists as well. I don't recommend that pattern in general because it is more complex to maintain than explicit dependencies, but it does give the ability to segment the application lifetime into various phases when required. When maintaining a startup / running / shutting / shutdown library, it can be difficult to have tracked everything unknown user code might have triggered, so you can just do a wait-for-everything to get everything into a known-good state. It isn't recommended (since it doesn't nest correctly), so it is usually better to do just about anything else. But application teardown is always just a bit messy, since all async operations must come to an end synchronously, which sullies the clean abstractions of the sync/async world. It is a bit like model dialog boxes: never good, but sometimes useful.

@lhames
Copy link
Contributor

lhames commented Oct 15, 2025

I've belatedly reverted the async actions patch in #163480. Closing this out.

@lhames lhames closed this Oct 15, 2025
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.

2 participants