Page MenuHomePhabricator

Expose rust native blob gRPC client API to C++
AbandonedPublic

Authored by marcin on Sep 5 2022, 11:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 7:36 AM
Unknown Object (File)
Mon, Nov 11, 4:20 PM
Unknown Object (File)
Sat, Nov 9, 1:01 AM
Unknown Object (File)
Fri, Nov 8, 4:00 PM
Unknown Object (File)
Fri, Nov 8, 12:18 AM
Unknown Object (File)
Tue, Nov 5, 11:27 AM
Unknown Object (File)
Tue, Nov 5, 11:05 AM
Unknown Object (File)
Tue, Nov 5, 11:05 AM

Details

Summary

This differential creates synchronous (called in a blocking manner on tokio runtime) versions of all streaming methods implemented so far and exposes them to C++ by cxx bindings.

Test Plan

To properly test this diff it is required to create some C++ code that uses rust methods implemented in this diff to upload and download some data to and from blob service. Currently it is hard to do so since we cannot just temporarily place C++ bindings for those rust methods somewhere in native code (AppDelegate.mm or CommCoreModule.cpp) since we cannot call rust from C++ on native yet. So in order to test this diff (in fact the entire diff stack) I created a simple branch with small C++ project that does exactly the same thing as test plan for parent differential. It can be seen here: https://github.com/CommE2E/comm/tree/marcin/blob-native-client-test-app.
To test one needs first to install corrosion. (https://github.com/CommE2E/comm/blob/master/services/lib/docker/install_corrosion.sh) and start blob service locally.
Then execute (from the root of blob_client directory) :

mkdir target
cd target
cmake ..
make
cd ..
./target/bin/blob_client_app

Any connection or transportation error will be logged to the console from try{...}catch(...){...} statement, and if data gest corrupted during transportation the assert statement should fail.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-384-native-rust-lib
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Sep 6 2022, 6:26 AM
tomek added inline comments.
native/cpp/CommonCpp/grpc/blob_client/rust/build.rs
4–5 ↗(On Diff #16334)

This code is already added in D5052 so this diff should be rebased.

native/cpp/CommonCpp/grpc/blob_client/rust/src/lib.rs
202 ↗(On Diff #16334)

It is really rare that &String is required. We should usually prefer &str, then String. Can we use &str here and in this whole file?

This revision now requires changes to proceed.Sep 6 2022, 6:26 AM
native/cpp/CommonCpp/grpc/blob_client/rust/src/lib.rs
202 ↗(On Diff #16334)

Yes, we can use &str in upload_chunk..... I will do the refactor.

native/cpp/CommonCpp/grpc/blob_client/rust/src/lib.rs
202 ↗(On Diff #16334)

I had discussion with @tomek and did some experimentation. We found that is it not safe to use &str here. The reason is that &str is mapped to ::rust::Str on C++ side (https://cxx.rs/binding/str.html). This structure has default destructor. (~Str() = default). If we pass std::string to a function that expects ::rust::Str the corresponding ::rust::Str instance will be constructed and once the function is done default destructor will be called that will try to delete C++ string it was instantiated with. It might result in memory error (we saw segmentation fault here). &String is mapped to ::rust::String&. ::rust::String has user-provided destructor and didn't result in segmentation fault.
Additionally using &str might be an issue if those function were exposed to another rust project, which is not the case here since those function are expected to be called exclusively from C++.
Therefore we will proceed with &String.

Updsate according to changes in differential eariler in the stack

Bring back localhost in server address

This revision is now accepted and ready to land.Sep 9 2022, 7:47 AM

Refactor after varun's changes

Diff from 2022 what I was assigned encrypted blob upload work.