Page MenuHomePhabricator

[keyserver] Adding Tunnelbroker-client napi-rs class constructor
ClosedPublic

Authored by max on Jan 30 2023, 2:29 AM.
Tags
None
Referenced Files
F3251524: D6459.id21710.diff
Fri, Nov 15, 5:38 PM
Unknown Object (File)
Thu, Nov 14, 4:34 AM
Unknown Object (File)
Fri, Nov 8, 3:10 AM
Unknown Object (File)
Fri, Nov 8, 3:10 AM
Unknown Object (File)
Fri, Nov 8, 3:10 AM
Unknown Object (File)
Fri, Nov 8, 3:10 AM
Unknown Object (File)
Fri, Nov 8, 3:10 AM
Unknown Object (File)
Fri, Nov 8, 3:10 AM
Subscribers

Details

Summary

This diff introduces a napi-rs JS class constructor for using the shared Tunnelbroker-client library in the keyserver's node codebase.
Using the napi-rs class constructor we can use the client as a standard class inside the keyserver's JS codebase.

The class constructor has two arguments:

  • Address where to connect to the Tunnelbroker;
  • JS Callback function which is called when the message in the input stream is received. The message payload is used as an argument to the callback function;

The constructor connects to the Tunnelbroker server, spawning a tokio task to listen for new messages from the stream and call the callback function on it.

Linear task: ENG-2729

Test Plan
  1. Keyserver service is successfully built.
  2. Manually testing:
    • Start the Tunnelbroker server;
    • Create a TunnelbrokerClient in a node js codebase by passing the address and a callback function.

The concept was tested using the echo bidi stream and simple client in the testing repo.

Diff Detail

Repository
rCOMM Comm
Branch
keyserver-add-napi-rs-client-class
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

max held this revision as a draft.
max edited the test plan for this revision. (Show Details)
max added reviewers: varun, jon.
max published this revision for review.Jan 30 2023, 3:11 AM
max edited the test plan for this revision. (Show Details)

Please ignore Codegen and ESLint fails, for now, because the failure seems doesn't relate to the keyserver (it's successfully built).

Any idea why the CI failures are occurring then?

Any idea why the CI failures are occurring then?

This is a protoc issue that we had before. I think it should be ok after D6411 will be re-landed.

Can you rebase your diffs on that one to address the CI failures?

Don't see any smells, would like bartek to take a look as well

max edited the test plan for this revision. (Show Details)

Rebasing.

Can you rebase your diffs on that one to address the CI failures?

Sure. Done, all CI gates are passed now.

varun requested changes to this revision.Jan 31 2023, 9:28 PM

just a couple questions inline. overall looks good

keyserver/addons/rust-node-addon/src/tunnelbroker_client.rs
22 ↗(On Diff #21710)

why are we passing address to the constructor? can't we just get this from an env var like we do in D5796?

23 ↗(On Diff #21710)

guessing we're passing a callback because the constructor can't be async?

This revision now requires changes to proceed.Jan 31 2023, 9:28 PM

Addressing comments:

  • Changes to use tunnelbroker service address from the environment variable.

Rebasing on parent changes.

max added inline comments.
keyserver/addons/rust-node-addon/src/tunnelbroker_client.rs
22 ↗(On Diff #21710)

why are we passing address to the constructor?

That seems like a common practice.

can't we just get this from an env var like we do in D5796?

I think it makes sense to use the same approach for all services. I've made changes to use the environment variable instead. Thanks, @varun !

23 ↗(On Diff #21710)

guessing we're passing a callback because the constructor can't be async?

Correct! Napi throws an error in case if the constructor is async.

This revision is now accepted and ready to land.Feb 2 2023, 8:34 AM
max marked 2 inline comments as done.

Rebasing on parent changes.

Reopening this diff due to solving ENG-3048.

This revision is now accepted and ready to land.Feb 21 2023, 6:06 AM