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

Reuse ActorContainerRef if it exists #2011

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Conversation

MellowYarker
Copy link
Contributor

Prior to this commit, if a Durable Object in Workerd (local dev/miniflare) hibernated (i.e. Worker::Actor was destroyed), but the stub wasn't dropped by the worker (i.e. PromisedWorkerInterface still alive), then we would incorrectly create an entirely new refcounted ActorContainerRef upon a new request coming in.

This is incorrect because the old ActorContainerRef is still owned, and by creating an entirely separate one, we break our request tracking.

Specifically, the ActorContainer only tracks a weak reference to an ActorContainerRef, and so if we have two separate ActorContainerRef's one of them will not be tracked.

Prior to this commit, if a Durable Object in Workerd (local
dev/miniflare) hibernated (i.e. Worker::Actor was destroyed),
but the stub wasn't dropped by the worker (i.e. PromisedWorkerInterface
still alive), then we would incorrectly create an entirely new refcounted
ActorContainerRef upon a new request coming in.

This is incorrect because the old ActorContainerRef is still owned, and
by creating an entirely separate one, we break our request tracking.

Specifically, the ActorContainer only tracks a weak reference to an
ActorContainerRef, and so if we have two separate ActorContainerRef's
one of them will not be tracked.
@MellowYarker MellowYarker marked this pull request as ready for review April 12, 2024 19:52
@MellowYarker MellowYarker requested review from a team as code owners April 12, 2024 19:52
@MellowYarker
Copy link
Contributor Author

I suspect this may fix #1422, although I haven't been able to force a segfault in server-test.c++ yet. At the very least, it's clear the code prior to this commit is dropping a live ActorContainerRef, and if the ActorContainer goes away before the untracked ActorContainerRef, then the untracked ActorContainerRef will attempt to access invalid memory in its dtor (which is what we see happening in that issue).

@MellowYarker MellowYarker merged commit fb6ad2f into main Apr 12, 2024
10 checks passed
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