Skip to content

Mark jsg::{Async}Generator visitable - #7213

Merged
jasnell merged 3 commits into
mainfrom
jasnell/fixup-memory-leaks-in-streams
Sep 3, 2026
Merged

Mark jsg::{Async}Generator visitable#7213
jasnell merged 3 commits into
mainfrom
jasnell/fixup-memory-leaks-in-streams

Conversation

@jasnell

@jasnell jasnell commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@jasnell
jasnell requested review from guybedford and npaun September 1, 2026 21:34
@jasnell
jasnell requested review from a team as code owners September 1, 2026 21:34
@jasnell
jasnell force-pushed the jasnell/fixup-memory-leaks-in-streams branch from ce4ee0b to 139199d Compare September 1, 2026 21:50
@ask-bonk

ask-bonk Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@jasnell Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@jasnell

jasnell commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

/bonk roast this PR

Comment thread src/workerd/api/streams/standard.c++
@ask-bonk

This comment was marked as resolved.

@jasnell
jasnell force-pushed the jasnell/fixup-memory-leaks-in-streams branch from 139199d to 937d28b Compare September 1, 2026 22:42
@jasnell

jasnell commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

/bonk ok, I believe that issue is resolved. check again

@ask-bonk

ask-bonk Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

LGTM!

github run

Comment thread src/workerd/api/streams/transform.c++

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Took a close look at the GC tracing changes and verified the behavioral surface of the IoContext/addFunctor removal empirically against main (notes inline). Overall the approach looks right.

One additional minor: TransformStreamDefaultController now keeps readableOwner/writableOwner rooted after error/terminate; they could be cleared at that point to release the wrappers earlier.

Comment thread src/workerd/api/streams/standard.c++
Comment thread src/workerd/api/streams/standard.c++ Outdated
Comment thread src/workerd/api/streams/standard.c++ Outdated
Comment thread src/workerd/jsg/iterator.h
Comment thread src/workerd/jsg/iterator.h Outdated
In certain edge cases the lack of gc visitation can lead
to memory leaks if certain promises go unresolved. There
was also an infinite loop in the gc trace for certain
TransformStrean constructions.

By making ThenCatchPair traceable, removing the unnecessary
addFunctor, and restoring JSG_VISITABLE_LAMBDA usage, we
resolve the issues
@jasnell
jasnell force-pushed the jasnell/fixup-memory-leaks-in-streams branch from 76ca2ad to 5908951 Compare September 3, 2026 14:05
@jasnell
jasnell merged commit 3721303 into main Sep 3, 2026
31 of 34 checks passed
@jasnell
jasnell deleted the jasnell/fixup-memory-leaks-in-streams branch September 3, 2026 15:54
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