Closed Bravo555 closed 1 month ago
codecov[bot]
commented
1 month ago Attention: Patch coverage is 79.90431% with 42 lines in your changes are missing coverage. Please review.
Project coverage is 77.9%. Comparing base (
644cd6f) to head (f5f7511). Report is 6 commits behind head on main.:exclamation: Current head f5f7511 differs from pull request most recent head 884b25f
Please upload reports for the commit 884b25f to get more accurate results.
github-actions[bot]
commented
1 month ago | :white_check_mark: Passed | :x: Failed | :next_track_button: Skipped | Total | Pass % | :stopwatch: Duration |
|---|---|---|---|---|---|
| 435 | 1 | 3 | 436 | 99.77 | 56m13.273314s |
| Name | Message | :stopwatch: Duration | Suite |
|---|---|---|---|
| Entities persisted and restored | ValueError: invalid literal for int() with base 10: '' | 65.819 s | Registration Lifecycle |
Bravo555
commented
1 month ago Closing in favour of #2904, which is a more promising and feasible approach.
Proposed changes
This PR should make the operation handle simpler to reason about and change, by allowing operation handlers to use
.awaitwithout blocking. The scope of the PR is to show the idea working on theconfig_snapshotandconfig_updateoperations.Below i describe the current state, which is subject to change.
Mutexes to guard theMqttPublisherandCumulocityConverter:struct MqttPublisher(Arc<Mutex<LoggingSender<MqttMessage>>>)withfn send(&self)is ok, as we lock the mutex to use the sender, and unlock it immediately after we finished sending, so it shouldn't block. The alternative would be to clone the sender for each worker, and I will explore it later.Mutex<CumulocityConverter>, it ensures that only a single worker can useCumulocityConverterat any given time, which can lead to deadlocks if we're not careful, and currently results in the same behaviour as before, i.e. blocking in the operation handling code will block processing of other messages, because the lock will be open across await points. This is temporary, and will have to be fixed, but is used right now so I could show how the full operation handling function would look like, without the current fragmentation. Also, the test demonstrating that the converter doesn't block should be made.Identitytype from thedownloadcrate, so I can use it in the next commit.Downloaderis used directly in the operation handler, to show how we can use it directly without going through the actor: if we only want to download a file via HTTP, and this download shouldn't influence anything else, we can use it directly.UploaderActorviaClientMessageBox, where we receive the response directly in the same place where wesend(), instead of going to the top-level receiver in the top-level actor, which then needs to decide to call an operation handling function again. This way we still use an uploader actor, and have messages sent between them, but don't need to fragment our control flow. This was done to show that we can still send and receive messages from other actors from within the proposed new operation handlers.Next steps
replace lock on mqtt sender with cloned senders(can be done, but will require&mut self)CumulocityConverterso operation handling doesn't blockTypes of changes
Paste Link to the issue
2880
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments