Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Reusable VM allows context leak due to global symbol properties #1592

Open
mjameswh opened this issue Dec 27, 2024 · 0 comments · May be fixed by #1519
Open

[Bug] Reusable VM allows context leak due to global symbol properties #1592

mjameswh opened this issue Dec 27, 2024 · 0 comments · May be fixed by #1519
Labels
bug Something isn't working

Comments

@mjameswh
Copy link
Contributor

Describe the bug

When the reuseV8Context option is enabled, the logic that swaps global variables in and out for a specific Workflow Execution only look at "regular" properties on globalThis, but ignores symbol properties. Symbol properties may therefore leak from one Workflow Execution to another.

In a recent investigation, this was identified as the cause of a memory leak when using Effect in Workflow Code, as Effect was storing its own "global store" as a symbol property on globalThis. Though this specific case has just been fixed on Effect's side, we recognize that there was nothing wrong in them using global symbol properties this way, and that other libraries are likely to do so. We must therefore ensure that Temporal SDK is able to properly handle global symbol properties.

Huge thanks to @izakfilmalter for his work in preparing a repro project for this issue, capturing heap dumps, and coordinating efforts between Effect's and Temporal's teams, as well as to @mikearnaldi from Effect for his precious collaboration in investigating this through.

@mjameswh mjameswh added the bug Something isn't working label Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant