This is a Rust fcm fcm notifications library wrapper to use the library in the C++ codebase to send push notifications to Android devices.
Linear task: ENG-1303
Details
Details
- Rust:
Run cargo build from services/tunnelbroker/rust-notifications directory.
Rust library will be successfully built.
- C++ (Docker):
Patch to D4807 (top of the stack) using arc patch D4807.
Running run run-tunnelbroker-service successfully built the Rust library and link it.
- C++ (Nix):
Patch to D4807 (top of the stack) using arc patch D4807.
Call cd services/tunnelbroker.
Running rm -dfr build && cmake -B build . && make -C build -j4 successfully built the Rust library and link it.
Library's FCM notifications push method can be called from the C++ as:
#include "rust_notifications/src/lib.rs.h"
#include "rust/cxx.h"
...
const unsigned long messageID = sendNotifToFCM(
char const *fcmAPIKey,
char const *deviceRegistrationID,
char const *messageTitle,
char const *messageBody
);Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- D4894
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/tunnelbroker/rust-notifications/src/fcm.rs | ||
|---|---|---|
| 19 ↗ | (On Diff #15809) | I don't like the usage of unwrap(), it essentially throws away a potential exception. We should really be inspecting the result, logging if there was an error, and then not causing a panic if it does return an Error. |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
| 75 ↗ | (On Diff #15809) | Seems like we are going from Result<FcmResponse> to bool denoting if it was Ok or Error. I think we should just be returning the Result value. Unless this is to be "more compatible" of a method to C++. In which the Result type doesn't really fit into the C++ "exception" landscape. |
Comment Actions
Use Anyhow and Result instead of bool.
Removing unwrap().
Using snake_case in Rust code, export to C++ as a camelCase.
Simplify the code.
| services/tunnelbroker/rust-notifications/src/fcm.rs | ||
|---|---|---|
| 19 ↗ | (On Diff #15809) |
This is a good point. I've switched to using Anyhow and Result. |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
| 75 ↗ | (On Diff #15809) |
Makes sens. I've changed it to return a Result with the message_id in case of success. |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
|---|---|---|
| 57–63 ↗ | (On Diff #16152) | can be written more minimally. Currently we're just unwrapping message_id with ?, and then re-wrapping it with Ok() |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
|---|---|---|
| 57–63 ↗ | (On Diff #16152) |
I wasn't sure about code readability to make it this way, because it can be harder to understand what the function returns to an upper level, and the rust-analyzer highlights only future here. |
| services/tunnelbroker/rust-notifications/src/fcm.rs | ||
|---|---|---|
| 10 ↗ | (On Diff #16212) | Why do we need to create a new client for every notification? |
| services/tunnelbroker/rust-notifications/src/fcm.rs | ||
|---|---|---|
| 10 ↗ | (On Diff #16212) |
The Client::new() is a reqwest::ClientBuilder. The HTTP request itself calls in Send. There is no point here to use a shared reqwest::ClientBuilder and reuse it because we will call this function from the different C++ threads and in this case, we will need to use a synchronization mechanism for the shared access and this will level out benefits and makes the code more complex on this simple call. Also, this can be a bottleneck in the case of the performance while parallel sending. |
| services/tunnelbroker/rust-notifications/src/fcm.rs | ||
|---|---|---|
| 10 ↗ | (On Diff #16212) | It seems like the main reason for not reusing the client is synchronization. Have you considered using a thread local approach? And as a separate thing, if we expect that a couple of clients will be sending at the same time, have we tested if it actually works? What is the limit of number of concurrent clients sending at the same time? |
| services/tunnelbroker/rust-notifications/src/fcm.rs | ||
|---|---|---|
| 10 ↗ | (On Diff #16212) |
The main reason here is that it will be a bottleneck in the case of parallel sending because we are using a synchronized single-client approach instead of scaling it according to the load. The benefit of reusing one client is overlapped by reducing scalability and adding complexity.
This function is blocked until the result is came. That's why it will be called in a separate thread in C++.
There is no limit, we spawn a new connection to FCM on every Send and close it. We can maintain a limit at the upper level of the tunnelbroker where we will call it. Something like an atomic counter with the limit in Constants. |