Details
- Keyserver service is successfully built.
- Manually testing:
- Start the Tunnelbroker server;
- Create a TunnelbrokerClient in a node js codebase by passing the address and a callback function.
- Call publish with the corresponding payload and device_id.
- The expected result is that the message will be delivered to the stream with the corresponding device_id.
The concept was tested using the echo bidi stream and simple client in the testing repo.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| keyserver/addons/rust-node-addon/src/tunnelbroker_client.rs | ||
|---|---|---|
| 63 ↗ | (On Diff #21558) | Unfortunately, NAPI-RS async functions in a class can be marked only as unsafe. |
Please ignore ESLint fails, for now, because the failure seems doesn't relate to the keyserver (it's successfully built).
This is a protoc issue that we had before. I think it should be ok after D6411 will be re-landed.
| keyserver/addons/rust-node-addon/src/tunnelbroker_client.rs | ||
|---|---|---|
| 63–71 ↗ | (On Diff #21558) | I would move the unsafe to just the block which needs it. Also avoids having to propagate the unsafe keyword to a call elsewhere |
| keyserver/addons/rust-node-addon/src/tunnelbroker_client.rs | ||
|---|---|---|
| 63–71 ↗ | (On Diff #21558) |
Unfortunately, the function itself should be marked as unsafe. When using your proposal: &mut self in async napi methods should be marked as unsafe
In our case, we must have a long-lived stream connection which is part of the class, and use the tx channel from the publish function. In this case, we should use this channel as a mut (actually we are not mutating anything, we are writing to the mpsc channel, but it's marked as mut). |
| keyserver/addons/rust-node-addon/src/tunnelbroker_client.rs | ||
|---|---|---|
| 63–71 ↗ | (On Diff #21558) |
unsafe best practices is bit outside of my comfort zone, so I'll take your word that this seems to be best current path forward. |
I see an async class method in the docs: https://napi.rs/docs/concepts/class#class-method
Are we sure we need unsafe here?
Unfortunately yes, because we are using mut &self here to access the stream in the class instance:
&mut self in async napi methods should be marked as unsafe.
In our case, we must have a long-lived stream connection which is part of the class, and use the tx channel from the publish function. In this case, we should use this channel as a mut (actually we are not mutating anything, we are writing to the mpsc channel, but it's marked as mut).
I can't see any other options on how to make this without unsafe here, this is a napi-rs limitation for using the shared structures.
I tried building your changes locally but I got the following error: error: &mut self is incompatible with async napi methods
The Sender (tx) can be cloned cheaply, so I was wondering if we could just clone to avoid having &mut self in the function signature
Did you rebase on the latest diff changes? I've successfully built using arc patch D6460 and then nix develop --accept-flake-config -c bash -c "cargo b".
The Sender (tx) can be cloned cheaply, so I was wondering if we could just clone to avoid having &mut self in the function signature
Despite that documentation saying that tx should be mut, I've did the trick and removed the mut from the parameter and it works without any errors and without unsafe. It's tested on the testing repo without unsafe and mut messages are successfully delivered, no any compiler errors, warnings.
We can now just remove mut and unsafe as well.
I've updated the diff to be without mut, unsafe, and cloning now, which is much much better. Thanks, @varun for a deep review!
I've dug into the napi Rust implementation and found that despite the docs saying that in this case I can try to omit mut+unsafe and it will work, and it's worked )