Page MenuHomePhabricator

[CommRustModule] improve error handling
ClosedPublic

Authored by kamil on Jan 4 2024, 3:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 4:53 AM
Unknown Object (File)
Sat, Nov 23, 12:35 AM
Unknown Object (File)
Fri, Nov 8, 5:11 PM
Unknown Object (File)
Sun, Nov 3, 8:27 PM
Unknown Object (File)
Oct 18 2024, 2:08 AM
Unknown Object (File)
Oct 18 2024, 2:08 AM
Unknown Object (File)
Oct 18 2024, 2:08 AM
Unknown Object (File)
Oct 18 2024, 2:08 AM
Subscribers

Details

Summary

Handle message form Identity and upload one-time keys on demand.

Addresses https://phab.comm.dev/D10374#inline-63986

Test Plan
  1. Add throw std::runtime_error("TEST ERRROR"); in one of methods
  2. Call method
  3. Verify that promise is rejected

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Jan 4 2024, 4:16 AM
native/cpp/CommonCpp/NativeModules/CommRustModule.cpp
259–274 ↗(On Diff #35196)

When I first looked at this code I realized that there is no resolve statement here since I guessed it must happen automatically somewhere else. So I asked @kamil whether it is possible that we will resolve before we throw. @kamil told that it can't happen since the only code that can throw here is addPromise call. Rust method won't throw and won't resolve promise if addPromise throws since it will never be executed then. That said this code is safe. Nevertheless I would find it much more readable if the try catch statement wrapped only addPromise call and reject it and early return if it throws.

This revision is now accepted and ready to land.Jan 4 2024, 10:34 AM
native/cpp/CommonCpp/NativeModules/CommRustModule.cpp
259–274 ↗(On Diff #35196)

as discussed in the office - it's better to keep it as it is right now

This revision was automatically updated to reflect the committed changes.