Unconditionally setup tasks for resource destructors - #14158
Conversation
fitzgen
left a comment
There was a problem hiding this comment.
r=me with nitpick below addressed
Also, do you want to have a disas test as well?
There was a problem hiding this comment.
Test coverage of the symmetric trapping cases? Probably worth exercising trapping before and after forcing the lazy thread.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We do clean up the VMDeferredThread stuff, which is what I was getting at: checking that if the resource destructor traps and then we later re-enter the store, we don't get stale VMDeferredThread stuff.
There was a problem hiding this comment.
Ah ok I see yeah, ok added some tests!
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.
8495c8d to
81c1ec8
Compare
|
I'm going to skip the disas test for now since the main purpose here is runtime functionality |
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.