Skip to content

Unconditionally setup tasks for resource destructors - #14158

Open
alexcrichton wants to merge 1 commit into
bytecodealliance:mainfrom
alexcrichton:resource-context-in-drop
Open

Unconditionally setup tasks for resource destructors#14158
alexcrichton wants to merge 1 commit into
bytecodealliance:mainfrom
alexcrichton:resource-context-in-drop

Conversation

@alexcrichton

Copy link
Copy Markdown
Member

This commit updates the translation of resource destructors in the component model to unconditionally setup a sync task as the spec specifies. This resolves an issue where context slots were leaking across boundaries when a component destroyed its own resource.

While here this updates the translation to use the inline fast path that is present in sync-to-sync adapters by juggling some code to make it sharable by the two locations.

This commit updates the translation of resource destructors in the
component model to unconditionally setup a sync task as the spec
specifies. This resolves an issue where context slots were leaking
across boundaries when a component destroyed its own resource.

While here this updates the translation to use the inline fast path that
is present in sync-to-sync adapters by juggling some code to make it
sharable by the two locations.
@alexcrichton
alexcrichton requested review from a team as code owners August 19, 2026 15:12
@alexcrichton
alexcrichton requested review from fitzgen and removed request for a team August 19, 2026 15:12

@fitzgen fitzgen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

r=me with nitpick below addressed

Also, do you want to have a disas test as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Test coverage of the symmetric trapping cases? Probably worth exercising trapping before and after forcing the lazy thread.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Could you expand on the trapping cases you're thinking of? The original intention for these tests was testing context.{get,set} in the context of resource destructors as opposed to trapping (where before it was just bad context set)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was thinking when the resource destructor itself traps, and then we do something else with the store to check that the thread context is cleaned up correctly for those new calls.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh we don't clean up anything on traps, not only here but in tons of places. That generally works out though because once an instance traps it can't ever be used again, so there can't be a test anyway for reusing a component after a trap.

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