Audit LifecycleWatch API shape

[ktoso]

Make sure it makes sense.

We'd like to implement it in terms of the ID where we store the watches INSIDE the ID actually rather than storing in the system whicih then does id -> lifecycle watch lookup perhaps even.

This is internal to the runtime; the protocol should expose watch/unwatch, people don't need to know it is implemented like that.

[ktoso]

On one hand it's pretty okey and watchTermination(of:) is quite nice, though it risks closing over the actor...

func something(worker: Worker) { 
    let otherState = ...
    watchTermination(of: worker) { terminatedID in 
        // DO NOT CAPTURE THE WORKER (weak is fine, but nonsense, it will always be nil when this triggers)
        // because if it is local you'll retain it and that'll keep it alive

       // very useful because we can capture
       log.info("\(terminatedID) terminated, other state: \(otherState)")
    }
}

So I don't really want to just drop this API... (I'm getting flashbacks of why we implemented "sub-receives" in behaviors to begin with, because the other style can be annoying for more complex things).

So perhaps we should not do this, and instead do:

distributed actor Watcher: LifecycleWatch { 

  // MUST IMPLEMENT:
  func actorTerminated(id: ActorID) async throws { 
     // implement handling here
     // We lost the ability to capture OTHER state though
     // which gives me flashbacks of why we did sub-receives in the first place
  }

But as I wrote this up, I thought:

• Perhaps the real answer is to offer BOTH; and invoke them both.
• And provide a default empty impl for actorTerminated?

I'd like some opinions here tbh @yim-lee @drexin @kavon @xedin @tomerd 🤔

[xedin]

watchTermination seems unsafe especially since worker is not supposed to be captured but it's too easy to make that mistake.

[yim-lee]

I think (of: worker) reads better than (id: ActorID), but this:

// DO NOT CAPTURE THE WORKER (weak is fine, but nonsense, it will always be nil when this triggers)
// because if it is local you'll retain it and that'll keep it alive

A lot of people will not know or will forget this gotcha, thus causing much headache. This is not limited to watchTermination of course, but any way to prevent it from happening would get my vote.

[kavon]

If the weakly captured worker in actorTerminated will always be nil because it's invoked during the actor instance's deinit, then I think it's too dangerous and unsafe to provide a reference to the actor from the deinit.

I think providing only the actor's ID to actorTerminated is the right approach for now. If there's a compelling reason to provide a reference to the instance itself, then that could be added later on.

[ktoso]

Agreed -- thank you everyone for chiming in, let's go for the safe shape. 👍