Skip to content

jsg::Promise::attach for spans#4861

Merged
danlapid merged 1 commit intomainfrom
dlapid/jsg_promise_attach
Aug 22, 2025
Merged

jsg::Promise::attach for spans#4861
danlapid merged 1 commit intomainfrom
dlapid/jsg_promise_attach

Conversation

@danlapid
Copy link
Copy Markdown
Collaborator

No description provided.

@danlapid danlapid requested review from a team as code owners August 22, 2025 00:19
@danlapid danlapid marked this pull request as draft August 22, 2025 00:20
@danlapid danlapid force-pushed the dlapid/jsg_promise_attach branch 2 times, most recently from 443d56a to 04ca90a Compare August 22, 2025 11:30
@danlapid danlapid marked this pull request as ready for review August 22, 2025 11:30
@danlapid danlapid force-pushed the dlapid/jsg_promise_attach branch from 04ca90a to d761d6a Compare August 22, 2025 12:11
@danlapid danlapid requested a review from jasnell August 22, 2025 12:29
jasnell
jasnell previously approved these changes Aug 22, 2025
Copy link
Copy Markdown
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

I'm a bit worried this is applying RAII patterns to GC'd objects.

JSG promises don't have a notion of cancellation. If the promise never resolves, the attachment might just be held forever or might be destroyed during GC at an unpredictable time. It would be unsafe to attach any I/O objects in this way unless they are wrapped in IoOwn (and I believe the TraceContext objects you're attaching are I/O objects, so this is broken).

Could the attachment itself hold GC references? If so this creates cycles. There doesn't appear to be any mechanism to make the attachment visitable.

@danlapid danlapid force-pushed the dlapid/jsg_promise_attach branch 3 times, most recently from 9961726 to 2ad04f5 Compare August 22, 2025 18:31
@danlapid danlapid force-pushed the dlapid/jsg_promise_attach branch from 2ad04f5 to cdc6056 Compare August 22, 2025 18:37
@jasnell jasnell dismissed their stale review August 22, 2025 19:05

Still not quite right...

jasnell

This comment was marked as resolved.

@jasnell jasnell dismissed their stale review August 22, 2025 19:30

Still not entirely convinced this is correct but after verifying for myself my concern here is cleared.

@danlapid danlapid force-pushed the dlapid/jsg_promise_attach branch from cdc6056 to d415d3b Compare August 22, 2025 20:50
@danlapid danlapid enabled auto-merge August 22, 2025 21:10
@danlapid danlapid disabled auto-merge August 22, 2025 22:55
@danlapid danlapid merged commit 7a714a1 into main Aug 22, 2025
20 of 23 checks passed
@danlapid danlapid deleted the dlapid/jsg_promise_attach branch August 22, 2025 22:55
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.

3 participants