This is a Rust a2 apns push notifications library wrapper for using the library in the C++ codebase to send the push notifications to iOS devices.
Linear task: ENG-1303
Details
Details
- Rust:
Patch to D4807 (top of the stack) using arc patch D4807.
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 apns notifications push method can be called from the C++ as:
#include "rust-notifications/src/lib.rs.h"
#include "rust/cxx.h"
...
const unsigned short responseCode = sendNotifToAPNS(
char const *certificatePath,
char const *certificatePassword,
char const *deviceToken,
char const *message,
bool const *sandbox
);Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- update-rust-notifs-wraper-with-cxx
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
The first line of crate documentation https://crates.io/crates/a2 says that
HTTP/2 Apple Push Notification Service for Rust using Tokio and async sending.
So this is only for APN, which is reflected in function name sendNotifToAPNS.
The Linear task is about handling all the notifs
Rust notification library integration into the C++ codebase including helper functions.
E.g. in a comment there
We should be able to communicate directly with APNs and FCM.
So what is the plan for FCM? I can't see any info about it.
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 9–12 | Is it something cxx-specific to use &String? I guess we should prefer using &str, then String, but &String should be used only when necessary. | |
| 30 | Why do we hardcode this value? | |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
| 37–40 | Why not to_string()? | |
| 46–59 | What about this? | |
Comment Actions
I've split APNS and FCM into two diffs, FCM will be a follow-up on this one. Sorry I didn't update this diff title and description.
Thanks, Fixed it!
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 9–12 |
I've missed this difference between String and str in case of performance we definitely must use &str here, according to this article also. | |
| 30 |
I think we should remove this two of set_sound and set_badge and go with the default for the notifications for the app. So I've removed them. | |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
| 37–40 |
I've changed to using just &str instead. | |
| 46–59 |
A good practice is to return at the end if possible, I would prefer to live it here. | |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
|---|---|---|
| 22 ↗ | (On Diff #15801) |
We are using the lazy initialization here. According to the tokio documentation, there are no parameters to new. Instead, we can use runtime builder, but I don't think we should define a number of workers, thread name, or thread stack in our scenario. |
| 35 ↗ | (On Diff #15801) |
According to the documentation this will block the current thread execution. The reason to block here is that we will call this from the C++ thread and will wait for a result there. In the case of non-blocking here we will have a future -> in the future results, because of calling the non-blocking future result from the C++ thread. The case with spawning and non-blocking seems reasonable when calling not from the C++ thread or just from the Rust app, or from C++ but from the blocking execution in case we don't want to block... Hope the answer is not messy. |
Comment Actions
Can we create an Error type for this library and avoid panicking? It's fine in the lazy_static block, but I don't like the use of unwrap elsewhere
Edit: Alternatively we can use anyhow. See this example: https://cxx.rs/binding/result.html#returning-result-from-rust-to-c
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 7 ↗ | (On Diff #15801) | what's wrong with snake case? |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
| 22 ↗ | (On Diff #15801) |
Curious why you think this |
| 32 ↗ | (On Diff #15801) | We can change the signature of this function to return a Result. See: https://cxx.rs/binding/result.html Gives us more flexibility with error handling than a bool |
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 7 ↗ | (On Diff #15801) | It doesn't look like the function is exposed in the ffi module, but if you're trying to follow C++ conventions, check this out: https://cxx.rs/attributes.html#rust_name-cxx_name |
Comment Actions
Using Anyhow and Result instead of bool as a return.
Remove unwrap()ing.
Using snake_case in Rust code, export to C++ as a camelCase.
Simplify the code.
Comment Actions
Good catch! I've changed to use Result and Anyhow. The code is much simpler with this approach by the way.
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 7 ↗ | (On Diff #15801) |
I've used our C++ naming convention with camelCase, but now it's removed in a favor of #[cxx_name = "..."] to export to C++ as a camelCase and use snake_case in a Rust. |
| 7 ↗ | (On Diff #15801) |
Wow! Thanks a lot @varun! |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
| 32 ↗ | (On Diff #15801) |
That's a good point to change! I've changed it to use Result and Anyhow. |
| services/tunnelbroker/CMakeLists.txt | ||
|---|---|---|
| 12 ↗ | (On Diff #16143) | This space is added by the Cmake linter. |
Comment Actions
Overall looks pretty standard for rust (at least from my intermediate knowledge of rust).
Would be nice to get some CI with tests involved so we can we can get away from having people do larger test plans (different issue).
I was unable to arc patch from master, but that's likely to be an issue which can be solved with a rebase.
Comment Actions
Android build fail isn't related to this diff:
error An unexpected error occurred: "https://registry.yarnpkg.com/setprototypeof/-/setprototypeof-1.2.0.tgz: Request failed \"503 Service Unavailable\"".
Comment Actions
Android build fail isn't related to this diff:
You should be able to click on the gate, then select "restart build" on the right hand side.
Comment Actions
Oh, I thought the state in phabricator will not update on this (it's linked to some unique id of the build run). That's cool, thanks!
Comment Actions
The tricky thing here is if you restart a job from the Buildkite UI it won't be reflected in Phabricator, but if you restart a build from Phabricator (via Harbormaster) it will be reflected
Comment Actions
That makes sense because Harbormaster will link to a new unique build id (or something like that). Thanks for a clarification @atul ;)
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 21–22 ↗ | (On Diff #16210) | Is it a recommended approach to create new client for every notification? Seems really wasteful. |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
| 22 ↗ | (On Diff #15801) |
@max this comment was probably missed. What about it? |
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 21–22 ↗ | (On Diff #16210) |
There is no point here to use a shared client 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. Same thing in D4894#146223 |
| services/tunnelbroker/rust-notifications/src/lib.rs | ||
| 22 ↗ | (On Diff #15801) |
According to the Tokio documentation using a Multi-Thread Scheduler is the best default point because of:
Using a current-thread-scheduler can make a problem when the Rust call spawns threads that don't need to be blocked and run in parallel. Seems to be in this case they can be blocked by each other. |
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 21–22 ↗ | (On Diff #16210) | Ok, but how about thread local? How expensive is it to create a new client? |
| services/tunnelbroker/rust-notifications/src/apns.rs | ||
|---|---|---|
| 21–22 ↗ | (On Diff #16210) |
I'm not a big fan of thread_local because we will copy the instance every time instead of creating which is quite light, but there is room for improvement. I've created a possible follow-up for this. Thanks @tomek ! |
Comment Actions
Android build error is unrelated to this diff (it doesn't touch any Android stuff), it fails with the downloading timeout.