refactor: Simplify actor request handlers by didier-wenzek · Pull Request #2600 · thin-edge/thin-edge.io

didier-wenzek · 2024-01-18T10:03:41Z

Proposed changes

In this PR, @Bravo555 rightly argued that using async/await would be a more maintainable way to handle concurrent download and upload tasks in the c8y mapper, compared to the current design where an actor has to explicit maintain the intermediate states of these concurrent tasks (i.e. sending a request to a server actor, persisting in the actor state the fact that a response is expected from that actor along all the data required to process the response, proceeding with other messages, finally receiving and processing the response with the restored state).

This PR proposes a way to better combine async/await tasks and actors.

The first two commits, e3034a6 and 83ee198, are preparation steps. The aim is to have tedge_actors::Sender which are not necessarily Clone.
A subsequent commit, 5c86395, cleans up the tedge_actors traits. This is currently the 4th commit, before merging I will have to move this commit in 3rd position.
The next step, 6b9e5d2e, is to wrap a oneshot::Sender into a tedge_actors::Sender,
which can be then used in RequestEnvelope<Request, Response>: messages wrapping a request with a tedge_actors::Sender to be used for the response.
The key step, 40538f5, is to deprecate the KeyedMessageBoxes and to replace them with boxes expecting RequestEnvelopes. A server has no more a fixed number of pre-registered clients nor a specific sender for its responses. Instead, the clients send a reply_to response sender along their requests.
The following commits dff9778 and 01ae9d6 (these will be squashed with the previous one on merge), applies the change to all the actors. The behavior is exactly the same has before. Only the way server responses are sent back has been changed under the hood.
~~One test is still failing and will have to be fixed: the timer actor behaves as expected on shutdown but the test fails to notice that the actor has been dropped as expected~~. Fixed by 5126f58

The POC is applied to the c8y_firmware_manager. The point is to remove the need for reqs_pending_download: HashMap<String, SmartRestFirmwareRequest> where are stored the data to resume the request handling once the firmware has been successfully downloaded. We want to replace this explicit state management with regular async/await code.

A first commit, aaff523, remove the cache but, doing so, introduce an issue.
As the firmware actor now awaits for the firmware to be fully downloaded before sending a message to the child device, the firmware actor ignores any other requests till the firmware is fully downloaded. This issue is captured by the handle_child_response_while_busy_downloading test, which is now failing. The goal of the following steps is to make this test pass again.
With ac9b7f7, the firmware actor struct is changed and splitted in two parts. The actor itself keeps only stateful things that cannot be shared (the message receiver and the cache of pending operations). All the channel senders are grouped in a struct Worker which is Clone and can be
spawned in background tasks.
After the previous stage, the FirmwareManagerWorker has no method and only holds data used by the FirmwareManagerActor. The following commit 1cd4f03 fixes that: are moved to the worker all the methods that doesn't access the actor state.
Finally, with 2f15275, the download task is launched in the background, fixing the expected behavior on concurrent requests and the handle_child_response_while_busy_downloading test.
In order to minimize the diff size the implementation of the FirmwareManagerWorker has been interleaved with the FirmwareManagerActor implementation. This will have to be fixed before merge.

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

github-actions · 2024-01-23T15:04:32Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
389	0	3	389	100	54m34.259999999s

crates/core/tedge_actors/src/channels.rs

crates/core/tedge_actors/src/servers/requests.rs

albinsuresh · 2024-03-07T06:10:12Z

crates/core/tedge_actors/src/servers/mod.rs

 }
+
+/// A connector to a [Server] expecting Request and returning Response.
+pub trait Service<Request: Message, Response: Message>:


This trait was added just for aesthetics, to look better when used in actor builders along with ServiceProviders, right?

If by aesthetics you mean a nicer type name, then yes. Also, the clients of a server actor doesn't need to know that their requests are wrapped into envelopes with a reply-to address.

Once this PR merged, we can go further. Indeed, the current ServiceProvider / ServiceConsumer abstraction can now be deprecated in favor of MessageSource / MessageSink.

albinsuresh · 2024-03-07T06:18:21Z

crates/extensions/c8y_http_proxy/src/lib.rs

-            ReplyToRequester,
-        >,
-        jwt: &mut impl ServiceProvider<RequestEnvelope<(), JwtResult>, NoMessage, ReplyToRequester>,
+        http: &mut impl Service<HttpRequest, HttpResult>,


Seeing that makes me appreciate that dummy Service abstraction a lot more, hiding all the unnecessary boilerplate like NoMessage, NoConfig etc.

That's the point of this abstraction. Not so dummy ;-).

albinsuresh · 2024-03-07T06:45:05Z

crates/extensions/c8y_firmware_manager/src/actor.rs

+        mut self,
+        operation_id: String,
+        smartrest_request: SmartRestFirmwareRequest,
+        mut input_receiver: mpsc::Receiver<FirmwareOperationResponse>,


Suggested change

mut input_receiver: mpsc::Receiver<FirmwareOperationResponse>,

mut response_receiver: mpsc::Receiver<FirmwareOperationResponse>,

I will do that in a final commit for this PR.

Done with a final commit: 0bd66fa

albinsuresh · 2024-03-07T07:01:51Z

crates/extensions/c8y_firmware_manager/src/actor.rs


-pub struct FirmwareManagerMessageBox {
-    input_receiver: LoggingReceiver<FirmwareInput>,
+struct FirmwareManagerWorker {


Suggested change

struct FirmwareManagerWorker {

/// A worker, spawned per request, that can handle a single firmware update request for a child device end-to-end

struct FirmwareManagerWorker {

Just trying to highlight that the scope of this worker is per request.

Done with a final commit: 0bd66fa

rina23q

I re-reviewed only c8y_firmware_manager. I have some trivial comments, so I would approve it.

rina23q · 2024-03-06T15:55:34Z

crates/extensions/c8y_firmware_manager/src/actor.rs

-    // 2. Operation timeouts from the TimerActor for requests for which the child devices don't respond within the timeout window
-    // 3. Download results from the DownloaderActor for firmware download requests
-
+    // 3. RequestOutcome sent back by the background workers once the firmware request has been fully processed or failed


Trivial ;)

Suggested change

// 3. RequestOutcome sent back by the background workers once the firmware request has been fully processed or failed

// 2. RequestOutcome sent back by the background workers once the firmware request has been fully processed or failed

Fixed: e37bc2e

rina23q · 2024-03-07T09:33:43Z

crates/extensions/c8y_firmware_manager/src/actor.rs

-                }
-                FirmwareInput::IdDownloadResult((id, result)) => {
-                    self.process_downloaded_firmware(&id, result).await?
+                FirmwareInput::OperationOutcome(outcome) => {


Ah nice, timeout moved to the worker!

rina23q · 2024-03-07T09:42:14Z

crates/extensions/c8y_firmware_manager/src/error.rs

+    },
+
+    #[error("Child device {child_id} did not respond within the timeout interval of {time_limit_sec}sec. Operation ID={operation_id}")]
+    FromTimeout {


(trivial) I think we use the From prefix for the errors inheriting from somewhere. Hence, here you don't need From.

Suggested change

FromTimeout {

ExceedTimelimit {

Fixed: e37bc2e

rina23q · 2024-03-07T09:50:49Z

crates/extensions/c8y_firmware_manager/src/tests.rs

    // Assert EXECUTING SmartREST MQTT message and FAILED SmartREST MQTT message due to missing 'cache' directory.
    mqtt_message_box
        .assert_received([
+            // FIXME: executing status is no more sent when download fails


You fixed it right?

Yes, this has been fixed by 2f10f93

This method was used only in 3 tests, while adding extra complexity. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

- The `Clone` trait is no more implemented by `DynSender` This was the main reason preventing the implementation of useful `Into` convertions between misc `DynSender`. - The `adapt()` function as been deprecated as this was a workaround the lack of `Into` convertions between misc `DynSender`. - The `sender_clone()` method is to be used both to `clone` and adapt a sender. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Instead of using an array of clients identified by their indexes, the client address is attached to the requests. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

This is an intermediate step. The next one will be to spawn a task to handle the download and subsequent steps so a the manager is not blocked during that time. The failing test (handle_child_response_while_busy_downloading) is a witness of this missing requirement. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

The idea is to have a worker which is Clone with methods that can be spawned in background tasks. The actor itself keeps only stateful things that cannot be shared (the message receiver and the cache of pending operations). This is an intermediate step. All the methods are for now still attached to the actor. The next step will be to move out from the actor to the worker all the methods that doesn't touch the actor state. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

…less methods Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Execute the whole firmware operation in a single async method. i.e. with no state cached in the actor. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek · 2024-03-07T13:33:59Z

Also noticed that the first commit message is wrong. That commit deprecated close_sender and not sender_clone as indicated by the commit message. sender_clone was deprecated in the second commit.

The commit message has been fixed.

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek had a problem deploying to Test Pull Request January 18, 2024 10:11 — with GitHub Actions Failure

didier-wenzek force-pushed the refactor/actor-request-handler branch from f2cae6d to 74dbfcf Compare January 23, 2024 14:38

didier-wenzek temporarily deployed to Test Pull Request January 23, 2024 14:45 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Auto January 23, 2024 14:51 — with GitHub Actions Failure

didier-wenzek force-pushed the refactor/actor-request-handler branch from 74dbfcf to 38a481e Compare February 1, 2024 14:48

didier-wenzek temporarily deployed to Test Pull Request February 1, 2024 14:56 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 1, 2024 15:27 — with GitHub Actions Inactive

didier-wenzek force-pushed the refactor/actor-request-handler branch from 38a481e to a59cb3a Compare February 7, 2024 13:46

didier-wenzek had a problem deploying to Test Pull Request February 7, 2024 13:54 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Pull Request February 7, 2024 18:19 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 7, 2024 18:21 — with GitHub Actions Inactive

jarhodes314 reviewed Feb 8, 2024

View reviewed changes

crates/core/tedge_actors/src/channels.rs Show resolved Hide resolved

crates/core/tedge_actors/src/servers/requests.rs Show resolved Hide resolved

didier-wenzek force-pushed the refactor/actor-request-handler branch from bf0eeeb to b1d30f9 Compare February 15, 2024 09:42

didier-wenzek had a problem deploying to Test Pull Request February 15, 2024 09:42 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request February 16, 2024 18:08 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request February 16, 2024 21:05 — with GitHub Actions Failure

didier-wenzek force-pushed the refactor/actor-request-handler branch from 0b8c25e to dff9778 Compare February 19, 2024 17:27

didier-wenzek temporarily deployed to Test Pull Request February 19, 2024 17:27 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 19, 2024 17:34 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Pull Request February 20, 2024 16:04 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Pull Request February 20, 2024 19:39 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 20, 2024 20:43 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Pull Request February 21, 2024 13:07 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 21, 2024 13:12 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Pull Request February 21, 2024 13:56 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto February 21, 2024 14:01 — with GitHub Actions Inactive

didier-wenzek marked this pull request as ready for review February 21, 2024 15:41

didier-wenzek requested review from albinsuresh and rina23q as code owners February 21, 2024 15:41

didier-wenzek requested review from Bravo555, albinsuresh, jarhodes314 and rina23q March 6, 2024 13:10

albinsuresh approved these changes Mar 7, 2024

View reviewed changes

rina23q approved these changes Mar 7, 2024

View reviewed changes

didier-wenzek had a problem deploying to Test Pull Request March 7, 2024 12:13 — with GitHub Actions Failure

didier-wenzek force-pushed the refactor/actor-request-handler branch from e37bc2e to ece233d Compare March 7, 2024 13:26

didier-wenzek had a problem deploying to Test Pull Request March 7, 2024 13:26 — with GitHub Actions Failure

didier-wenzek added 10 commits March 7, 2024 14:29

Deprecate Sender::sender_close()

fb1650d

This method was used only in 3 tests, while adding extra complexity. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Split trait Sender into Sender + CloneSender

d64e795

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Use oneshot sender for server responses

f347e57

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Deprecate keyed message boxes

d3abd9a

Instead of using an array of clients identified by their indexes, the client address is attached to the requests. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Move out from FirmwareManagerActor to FirmwareManagerWorker all state…

1d8ca94

…less methods Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Launch firmware download in the background

bb9f16c

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Remove c8y firmware dependency on tedge_timer_ext

917e3e1

Execute the whole firmware operation in a single async method. i.e. with no state cached in the actor. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek force-pushed the refactor/actor-request-handler branch from ece233d to 917e3e1 Compare March 7, 2024 13:32

didier-wenzek temporarily deployed to Test Pull Request March 7, 2024 13:32 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto March 7, 2024 13:37 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Pull Request March 7, 2024 14:16 — with GitHub Actions Inactive

Move FirmwareManagerWorker in a worker module

0bd66fa

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek force-pushed the refactor/actor-request-handler branch from 21864b4 to 0bd66fa Compare March 7, 2024 14:20

didier-wenzek temporarily deployed to Test Pull Request March 7, 2024 14:21 — with GitHub Actions Inactive

didier-wenzek temporarily deployed to Test Auto March 7, 2024 14:26 — with GitHub Actions Inactive

didier-wenzek changed the title ~~Refactor the actor request handlers~~ refactor: Simplify actor request handlers Mar 7, 2024

Bravo555 mentioned this pull request May 15, 2024

Simplify the operation handling code #2880

Open

	mut input_receiver: mpsc::Receiver<FirmwareOperationResponse>,
	mut response_receiver: mpsc::Receiver<FirmwareOperationResponse>,

	struct FirmwareManagerWorker {
	/// A worker, spawned per request, that can handle a single firmware update request for a child device end-to-end
	struct FirmwareManagerWorker {

	// 3. RequestOutcome sent back by the background workers once the firmware request has been fully processed or failed
	// 2. RequestOutcome sent back by the background workers once the firmware request has been fully processed or failed

Conversation

didier-wenzek commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

github-actions bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rina23q left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek commented Mar 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

didier-wenzek commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 23, 2024 •

edited

Loading

didier-wenzek Mar 7, 2024 •

edited

Loading

didier-wenzek Mar 7, 2024 •

edited

Loading

didier-wenzek Mar 7, 2024 •

edited

Loading