tonarino / actor

A minimalist actor framework aiming for high performance and simplicity.
MIT License
40 stars 6 forks source link

Missing address warning #82

Closed mbernat closed 1 month ago

mbernat commented 1 month ago

This PR adds a warning when a SpawnBuilder is used without an address and with a default capacity. Such usage is often erroneous and it's hard to debug why such an actor is not receiving messages coming to an unassigned address.

Alternatives:

The other changes in this PR are fixing clippy.

mbernat commented 1 month ago

@PabloMansanet @strohel thanks for the feedback! I'm glad you both suggest hard errors, even though they are technically semver breaking. I think either approach fixes the problem, so I'll implement whichever is easier, although it'd be desirable to have both.

strohel commented 1 month ago

Another possibility is to add a warning on the sending side, if an address wasn't connected to an actor.

Instead, the solution I was thinking about was similar to your second "alternative": it would be to hard-error when a message is sent to an address that's not associated to any actor, either because it wasn't passed in to the SpawnBuilder, or because it didn't come from a default spawn.

This may be hard to do, because it is a valid use-case to create an Addr, send to messages to it, and only then spawn an actor that processes these messages.

Otherwise it would be tricky to create some actor systems with circular dependencies.

PabloMansanet commented 1 month ago

Another possibility is to add a warning on the sending side, if an address wasn't connected to an actor.

Instead, the solution I was thinking about was similar to your second "alternative": it would be to hard-error when a message is sent to an address that's not associated to any actor, either because it wasn't passed in to the SpawnBuilder, or because it didn't come from a default spawn.

This may be hard to do, because it is a valid use-case to create an Addr, send to messages to it, and only then spawn an actor that processes these messages.

Otherwise it would be tricky to create some actor systems with circular dependencies.

That's a good point.

I wonder if we should zero in on the actual issue that we had: the queue overflowed because there was no actor handling it. Maybe it suffices to add a special warning/error message to the queue overflow, in the cases that Addr has never been associated to a spawned actor.

Something like "Queue overflowed. No actor has been spawned with than address, make sure you didn't mix up the addresses", or similar.

mbernat commented 1 month ago

Yeah, I tried doing something on the address side and it's non-trivial. Also, our current design is in a kind of a local optimum where Addr is not technically an address but contains both ends of the channel but it publicly derefs to the sender sides (Recipient) and privately exposes the receiver sides only to the actor framework. But you could probably still misuse it by assigning the same address to two different actors.

It's hard to see how to improve on this design without major changes. We'd need to attach some extra state to it using interior mutability and it feels much worse than what we have right now.

strohel commented 1 month ago

Right, I think the only change that could be worth it now would be on the SpawnBuilder side.

More specifically, we would have a 2-stage builder. The first stage without any addr or capacity, and the second stage with addr. The only way to go from the first stage to the second would be to use (exactly once)

This would be indeed semver-breaking, but most (if not all) valid usages should still compile without any changes.

mbernat commented 1 month ago

Closing this in favor of new PR with the API proposed by @strohel .