Feat/request cancel support

[xgroleau]

Fixes #11

Note that the previous implementations was removed, we could keep them and add the cancel limitation in the comments.

It's starting to get a bit unwieldy for the typing when things need to specify the types and generics are not supported (ex: embassy tasks).

ActoRequest now uses a mutable reference to avoid doing two request from the same ActorRequest and joining them since the first reply could be associated with the second request for example.

[lulf]

This approach works I think, left one comment on handling cancelled requests.

[lulf]

Would it be better to attempt draining the channel using try_recv()? Or do we always know that the other side will send the reply?

[xgroleau]

Good question, I didn't think of draining the channel. Thing is we might not receive the response associated with the right request.

Here's an example of what could happen

• Sender: Sends a request 1
• Actor: Starts handling request 1 but it takes some time
• Sender: Cancels request 1
• Sender: Sends request 2 (drains channel, but nothing in it yet)
• Actor: Completes request 1
• Sender: Receives response from request 1 instead of 2

We can either

• Always assume a response is sent back
• Or assume that is not the case and accept that we might receive the response from the previous request when doing cancellation
• Or have a way to let the Request struct know that request was cancelled and no send the response when the request was cancelled. Would probably be ideal but I don't know yet how we could implement this.

[lulf]

@xgroleau I admit I totally forgot about this. I think it's ok to merge if you're happy with it the way it is.

[xgroleau]

@xgroleau I admit I totally forgot about this. I think it's ok to merge if you're happy with it the way it is.

No worries, honestly I did too.

I just completed the integration of the new changes in our code base, but I feel like the types and macro usage is starting to get in the way of readability a bit. I wonder if there would be a better way to handle this.

Actual example in our codebase for spawning ONE actor. we probably have a dozen.

   let battery_monitoring = BatteryMonitoringActor::new(
       comm_addr,
       ector::request!(log_addr, Result<&'static Path, StorageError>), // Since it handles Requests
       ector::request!(metrics_addr, Result<&'static Path, StorageError>), // Also here
       gui_addr,
       ector::request!(power_management_addr, BatteryState), // and here
   );

   ector::spawn_context!(
       BATTERY_MONITORING_ACTOR,
       med_spawner,
       battery_monitoring_actor,
       BatteryMonitoringHandler, // Type alias for the full type
       battery_monitoring
   );

It does work, I just don't like the fact that I need to specify the return type of the Request in the macro and might not be obvious what's going on for a new contributor.

If it's too much, we could make it "safe" by panicking if a request is dropped while it's ongoing to avoid writing on the dropped channel. It would also allow the request address to be Clone and impl From<Address<.....>> for RequestManager<....>instead of needing static allocation for each RequestManager.

Not entirely sure what's best honestly. Curious what you think about it @lulf .

[lulf]

I agree readability suffers a bit now, and also I find macros reduce readability even more. I don't have any good ideas, but maybe panicking for the edge cases is better to simplify

[xgroleau]

Yea that's what I also feel like. I'll come up with another PR for panicking when a request is cancelled.

[xgroleau]

Cancelled in favor of #14