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

Implement a weak version of after #1467

Closed
dominiklohmann opened this issue Jul 3, 2023 · 6 comments
Closed

Implement a weak version of after #1467

dominiklohmann opened this issue Jul 3, 2023 · 6 comments

Comments

@dominiklohmann
Copy link
Contributor

Using caf::after is dangerous: It permanently keeps a strong reference to the actor on its own clock, leading to an actor never shutting down because its handle's refcount going to zero.

I propose adding a version of caf::after that holds a weak reference to the actor, or—which I would personally prefer—to make the breaking change of always holding a weak reference in caf::after.

CAF is currently designed in a way that actors should shut down when their refcount goes to zero. However, some mechanisms in CAF actively work against that, and as such are easy traps to fall into for new users especially.

@Neverlord
Copy link
Member

I do like your set_idle_handler idea from #1590 (comment). But I think there's are two modifiers: strong/weak and repeated/single-shot.

What about this idea?

// strong ref, called once
self->when_idle(1s).once([] {...});
// weak ref, called indefinitely until replaced or canceled
self->when_idle(1s, weak).repeat([] {...});

Both versions would return a disposable that allows you to cancel the idle callback.

@dominiklohmann
Copy link
Contributor Author

dominiklohmann commented Jan 26, 2024

I like the UX of once/repeat. For strong/weak, however, I think it should just be weak. Making the strong version the default is a dangerous default, and if a user knows what they're doing and they want to opt into letting the actor keep a strong reference to itself, then they should probably do so explicitly:

using foo_actor = typed_actor<...>;
// Given a `foo_actor::pointer self`:
self->when_idle(1s).once([self, keepalive=foo_actor{self}] {
  // ...
});

The same also applies to self->run_delayed, for which I also think that the current defaults are dangerous.

@Neverlord
Copy link
Member

I think having weak as default would be a surprising source of error for many users. I know you're quite adamant about it, but I think most use cases actually want strong. Passing an extra, unused handle to the lambda doesn't feel like a good API pattern to me.

However, I wouldn't mind just making it explicit in all cases, i.e., not provide a default at all. Then, you'd have to write either when_idle(1s, strong) or when_idle(1s, weak). Then you'll always get what you're asking for.

@dominiklohmann
Copy link
Contributor Author

I think from a user perspective you'd expect once to keep alive, but repeat not to. So maybe it should be when_idle(t).{once,repeat}_{weak,strong}(fn) to make it very explicit?

@Neverlord
Copy link
Member

I think we agree on the semantics. However, we'll implement this as set_idle_handler (#1789). The name is a bit more verbose, but it makes clear that there can only be one such handler and it's more in line with the other "special" handlers.

@dominiklohmann
Copy link
Contributor Author

Awesome, thank you. It's really nice to see all these usability improvements lately leading up to the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants