chore: ntx builder actor deactivation#1705
Conversation
SantiagoPittella
force-pushed
the
santiagopittella-ntx-code-organization
branch
from
e8864fa to
712ece2
Compare
SantiagoPittella
force-pushed
the
santiagopittella-ntx-builder-actor-deactivation
branch
from
c30c953 to
d56b50b
Compare
SantiagoPittella
force-pushed
the
santiagopittella-ntx-code-organization
branch
from
712ece2 to
acacbf3
Compare
SantiagoPittella
force-pushed
the
santiagopittella-ntx-builder-actor-deactivation
branch
from
d56b50b to
ac9086b
Compare
SantiagoPittella
force-pushed
the
santiagopittella-ntx-code-organization
branch
from
acacbf3 to
9bede8d
Compare
SantiagoPittella
force-pushed
the
santiagopittella-ntx-builder-actor-deactivation
branch
from
75f97f5 to
ebf9d91
Compare
| ActorShutdownReason::IdleTimeout(account_id) => { | ||
| tracing::info!(account_id = %account_id, "Account actor shut down due to idle timeout"); | ||
|
|
||
| // Remove the actor from the registry, but check if a notification arrived | ||
| // just as the actor timed out. If so, the caller should respawn it. | ||
| let should_respawn = | ||
| self.actor_registry.remove(&account_id).is_some_and(|handle| { | ||
| let notified = handle.notify.notified(); | ||
| tokio::pin!(notified); | ||
| notified.enable() | ||
| }); | ||
|
|
||
| Ok(should_respawn.then_some(account_id)) |
There was a problem hiding this comment.
This works, but I really still think we shouldn't need ActorShutdownReason. The actor return value can just be a anyhow::Result<()>. You'll also notice that next returns a result, but we never actually return a result.
There was a problem hiding this comment.
We use the ActorShutdownReason in the coordinator side to determinate if we should re-spawn it
There was a problem hiding this comment.
Yes but there is only one reason to respawn it (currently, and for the foreseeable future imo)
- if the actor itself decides to shutdown, and
- if the actor had unhandled notifications
- then we should restart it
An actor currently shuts down (on purpose) if:
- it idles for too long
- its creation tx was reverted
Everything else is a crash/error/bug.
We can therefore in theory say that all Ok(()) means check notify and possibly respawn, and all Err(_) means increment crash counter and respawn.
I'm okay with merging as is, and we can litigate/discuss after. Basically I want to reduce the number of branches and conditionals because these lead to edge cases and bugs.
There was a problem hiding this comment.
Oh ok, I misunderstood before. It should be a simple change. To avoid generating conflicts in this PR #1712 I will address this there since it is related to that PR too
SantiagoPittella
force-pushed
the
santiagopittella-ntx-code-organization
branch
2 times, most recently
from
24eca2c to
429b63e
Compare
SantiagoPittella
force-pushed
the
santiagopittella-ntx-builder-actor-deactivation
branch
from
ebf9d91 to
870292b
Compare
Mirko-von-Leipzig
deleted the
santiagopittella-ntx-builder-actor-deactivation
branch
SantiagoPittella
restored the
santiagopittella-ntx-builder-actor-deactivation
branch
SantiagoPittella
deleted the
santiagopittella-ntx-builder-actor-deactivation
branch
2 participants
Closes the third task of #1694