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

Fix task dead cancelation race condition #2444

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ealmloff
Copy link
Member

@ealmloff ealmloff commented May 23, 2024

#2440 is caused by restarting a future that has already finished. Something like this was happening:

  1. resource runs (TaskId(0))
  2. resource finishes. The task id is recycled
  3. A new task is spawned for the async event handler (TaskId(0))
  4. Inside that task, the resource cancels its old future that was already recycled TaskId(0)
  5. The resource also spawns a new future in an empty id. TaskId(0) which we are currently in was already recycled so the new future is again spawned in TaskId(0)
  6. The async event handler finishes and since the future is resolved it removes the task. This removes TaskId(0)
  7. Now the second run of the resource which was spawned on TaskId(0) has already been dropped so it is never polled

This PR fixes that issue by switching the resource's task to None when the future is finished. It also applies the fix to use_future and use_coroutine.

Fixes #2440

@ealmloff ealmloff added bug Something isn't working core relating to the core implementation of the virtualdom breaking This is a breaking change labels May 23, 2024
@ealmloff ealmloff added the backport PRs that should be back ported to the last release label Jun 6, 2024
@ealmloff ealmloff removed the backport PRs that should be back ported to the last release label Jun 7, 2024
@jkelleyrtp
Copy link
Member

jkelleyrtp commented Jun 7, 2024

Can we use the current_scope from the runtime to get this information rather than overloading the component functions with the threadlocal? I'm just worried about panics/unwinding or other effects leaving things in a weird state between two threadlocals - would prefer to keep it all on the one runtime threadlocal.

We should have a current_scope function on the runtime somewhere that we can check the name/typeid of the current component against the one we're calling.

Discord convo:

Evan Almloff
It shouldn't be public either way. current_component is only sometimes set and only in debug mode
I don't love having a bunch of fields on the runtime that are disabled in release mode, but it doesn't matter that much
Jon Kelley — Today at 1:18 PM
We could make it public but just don't do the check in release
We would still be setting the value
I believe we already set the ScopeId somewhere?
rendering field on runtime could become Cell<Option<ScopeId>> for the same cost
and then the name/typeid query would be an indirection
        self.runtime.scope_stack.borrow_mut().push(scope_id);
Evan Almloff — Today at 1:21 PM
Yeah, we set the currently running scopeid right now for parents
Jon Kelley — Today at 1:22 PM
Okay, no rush but I'll put this convo on the PR itself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change bug Something isn't working core relating to the core implementation of the virtualdom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource does not restart from async event handler
2 participants