Modify Actor trait to take self instead of &mut self

[jarhodes314]

Proposed changes

In #2327, some changes have been made that assume we only run MqttActor::run once. As far as I can tell, we only ever want to run actors once, but some complexity around calling consuming methods (i.e. those which take an owned self argument) from Box<dyn Actor> prevented us from doing that. I've managed to work around this and modified Actor::run to receive self instead of &mut self.

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
• Documentation Update (if none of the other choices apply)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA (in all commits with git commit -s)
• I ran cargo fmt as mentioned in CODING_GUIDELINES
• I used cargo clippy as mentioned in CODING_GUIDELINES
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Further comments

[codecov]

Codecov Report

Merging #2329 (f0f7f37) into main (d39c1c2) will increase coverage by 0.0%.
The diff coverage is 95.5%.

Additional details and impacted files

Files	Coverage Δ
crates/common/batcher/src/driver.rs	77.4% <100.0%> (ø)
crates/core/tedge_actors/src/actors.rs	75.2% <100.0%> (+0.7%)	⬆️
crates/core/tedge_actors/src/builders.rs	69.2% <ø> (ø)
crates/core/tedge_actors/src/converter.rs	53.9% <100.0%> (ø)
crates/core/tedge_actors/src/run_actor.rs	100.0% <100.0%> (ø)
crates/core/tedge_actors/src/runtime.rs	86.6% <100.0%> (ø)
crates/core/tedge_actors/src/servers/actors.rs	87.0% <100.0%> (ø)
crates/core/tedge_actors/src/servers/builders.rs	71.7% <100.0%> (ø)
crates/core/tedge_actors/src/tests.rs	80.0% <100.0%> (ø)
...core/tedge_agent/src/file_transfer_server/actor.rs	86.4% <100.0%> (ø)
... and 29 more

... and 2 files with indirect coverage changes

[github-actions]

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
319	0	3	319	100	47m34.737999999s

[didier-wenzek]

Nice, to see this move. I can only encourage you to move this forward.

As far as I can tell, we only ever want to run actors once, but some complexity around calling consuming methods (i.e. those which take an owned self argument) from Box prevented us from doing that.

I confirm that we only want to run actors once.

I've managed to work around this and modified Actor::run to receive self instead of &mut self.

Running an actor as self was indeed the original design, but has been changed to &mut self for reasons that no more apply as meantime the message box design has been simplified.

[didier-wenzek]

Both fn slip and struct MqttMessageBox can be deprecated as built & split only here.

[jarhodes314]

I don't quite understand what you mean here. I introduced split as a mechanism to enable concurrent processing of incoming and outgoing messages, without the need for other actors to know/care about this detail (so outside there is still a single message box for a single actor). MqttActor could potentially be split into two actors to deal with this, but that feels like it's just going to increase complexity in all the MQTT interactions for other actors.

[didier-wenzek]

I meant that MqttActor doesn't need to define a message box as a single object. In fact no actor has to. The abstraction with other actors is implemented by the builder with appropriate ServiceProvider / MessageSource implementation.

The following is working fine:

pub struct MqttActor {
    mqtt_config: mqtt_channel::Config,
    from_peers: FromPeers,
    to_peers: ToPeers,
}

[didier-wenzek]

I pushed a fixup commit: cc067c1

[didier-wenzek]

Approved. This really clarify the intent of Actor::run().

[albinsuresh]

Approved. Surprised to see that even the changes required in the runtime module were so minimal.

[albinsuresh]

I was wondering why you didn't go with the simpler fix of self: Box<Self> (suggested in that same forum discussion) here instead. I'm assuming you did that to make the run method implemented by the actors look more natural and hide the complexity of Box<Self> with that blanket impl of ActorBoxed below.

[jarhodes314]

The existing API works well for our use case, and I really don't like tests polluted with boxing values simply because the actual code uses dynamic dispatch somewhere.

Box<Self> is very much an implementation detail here, and I want to be clear about that, particularly given the main change that's actually being made here is to the communication of what run means and what assumptions can be made when implementing it, which up until now have been implicit. Receiving Box<Self> doesn't tell me anything useful as an implementor of the trait, and because it's pretty uncommon thing, it's likely to cause some confusion among people.