[NativeRustLibrary] support `std::promise`
ClosedPublic
Actions

Authored by kamil on Jan 11 2024, 10:04 AM.

Details

Reviewers

marcin
michal
varun

Commits

rCOMMc98e25d3f6a7: [NativeRustLibrary] support `std::promise`

Summary

Context in ENG-6384

I tried to implement it using the template (std::promise<T>) but this complicates the code. I think we can assume the caller will handle it properly.

Test Plan

Test both rejecting/resolving
I used one of existing methods and added sleep in Rust code to wait long enough and make sure it works

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Jan 11 2024, 10:04 AM

kamil held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 11 2024, 10:04 AM

kamil edited the summary of this revision. (Show Details)Jan 11 2024, 10:05 AM

kamil retitled this revision from [NativeRustModule] support `std::promise` to [NativeRustLibrary] support `std::promise`.

Harbormaster completed remote builds in B25715: Diff 35556.Jan 11 2024, 10:28 AM

kamil published this revision for review.Jan 11 2024, 10:33 AM

you tested that both promise types work?

native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.cpp
47–49 ↗	(On Diff #35556)	i introduced this code originally but it looks wrong to me now. we probably shouldn't be adding JSI promises to `promises` if the jsInvoker is missing

This revision now requires changes to proceed.Jan 12 2024, 11:58 AM

ashoat added inline comments.Jan 12 2024, 1:52 PM

native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.cpp
8 ↗	(On Diff #35556)	Can you explain what’s going on with the formatting changes?

varun added inline comments.Jan 12 2024, 2:33 PM

native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.cpp
8 ↗	(On Diff #35556)	This is consistent with `CommConstants.cpp` (which was introduced afterwards, but uses the better style IMO)

ashoat added inline comments.Jan 13 2024, 6:07 PM

native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.cpp
8 ↗	(On Diff #35556)	Shouldn't the style be set by `clang-format`?

address review

In D10613#307308, @varun wrote:

you tested that both promise types work?

yes, in D10631 where we use both, C++ for prekey rotation and JSI to log in to Identity (D10327)

native/cpp/CommonCpp/NativeModules/InternalModules/RustPromiseManager.cpp
8 ↗	(On Diff #35556)	I manually ran `clang-format` so now it is, not sure why previously it wasn't (it should have been formatted last time the file was committed)
47–49 ↗	(On Diff #35556)	fixed