In this diff, I've created a new singleton called RustPromiseManager. This singleton is responsible for keeping track of unresolved promises that depend on the execution of async Rust code, and for eventually rejecting or resolving the promises.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.h | ||
|---|---|---|
| 3 ↗ | (On Diff #23910) | |
| native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.h | ||
|---|---|---|
| 3 ↗ | (On Diff #23910) | i should actually remove this include altogether |
Comment Actions
The test plan is D7114, but that diff failed the Android build. Can you fix it up so that the Android build is no longer breaking? I'm also curious how you were able to test the Android build... was it succeeding in your environment despite failing in CI?
| native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.h | ||
|---|---|---|
| 3 ↗ | (On Diff #23910) | Yeah it can be removed |
| 19–65 ↗ | (On Diff #23910) | These implementations should be in the .cpp file. I suspect you put them here to reduce the amount of copy-paste, but the real question is why you're doing any copy-paste in the first place |
| native/ios/Comm/RustPromiseManager.mm | ||
| 1–27 ↗ | (On Diff #23910) | I don't understand why you implemented two 100%-identical files, one for iOS and one for Android. Can you dedup please? You can use a .cpp filename suffix and store in native/cpp/CommonCpp/NativeModules/InternalModules. (Separately, this is the kind of thing that you should expect your reviewer to call you out on. If you have a good reason for doing this, it will always shorten review time to annotate that reasoning ahead of time instead of waiting for your reviewer to request changes.) |
Comment Actions
Collection should be threadsafe
| native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.h | ||
|---|---|---|
| 37 ↗ | (On Diff #24098) | |
Comment Actions
Talked to @varun offline, plan is to handle thread safety in a later diff
| native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.cpp | ||
|---|---|---|
| 24–34 ↗ | (On Diff #24098) | I think we can make this cleaner by exiting earlier: auto it = promises.find(id); if (it == promises.end()) { return; } |
| 39 ↗ | (On Diff #24098) | Same here |
Comment Actions
sorry, requesting review again because i decided to just add the thread safety changes to this diff....
Comment Actions
Looks great, just minor nits! Great work on the synchronization, really for the distraction looking at threadsafe collections
| native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.cpp | ||
|---|---|---|
| 3 ↗ | (On Diff #24101) | Nit: add a newline |
| native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.h | ||
| 12 ↗ | (On Diff #24101) | Ehh personally I don't love this in a .h file. Don't think it's as bad in a .cpp file, eg. see here. This is a nit though |
| 40 ↗ | (On Diff #24101) | Nit: add a newline |