Page MenuHomePhabricator

[services] Tunnelbroker - Fix calling blocking C++ function from an async task
ClosedPublic

Authored by max on Feb 23 2023, 3:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 16, 12:50 AM
Unknown Object (File)
Tue, Dec 10, 8:42 AM
Unknown Object (File)
Oct 27 2024, 5:51 PM
Unknown Object (File)
Oct 21 2024, 3:40 PM
Unknown Object (File)
Oct 18 2024, 5:41 PM
Unknown Object (File)
Oct 18 2024, 5:41 PM
Unknown Object (File)
Oct 18 2024, 5:40 PM
Unknown Object (File)
Oct 10 2024, 11:03 AM
Subscribers

Details

Summary

This diff fixes the calling of the blocking C++ function from an async Tokio task. Without this fix the scheduler first will run the blocking C++ function in a loop and put the async tasks in a queue after the blocking functions in a c++ thread. This causes an issue where the mpsc receiver will invoke a calling function only when the buffer size + 1 instead of on every push to the mpsc channel. As a result, the Tonic server will send messages to the stream only when the buffer size + 1 instead of every new message.

Full context in a linear task: ENG-3065

Test Plan
  1. Start the Tunnelbroker server.
  2. Using gRPC client (i.e. BloomRPC) connect to the Tunnelbroker messages stream as a client 1.
  3. Using gRPC client (i.e. BloomRPC) connect to the Tunnelbroker messages stream as a client 2 in a separate tab.
  4. Send one message to client 2 from client 1.
  5. Client 2 will receive the message from client 1 in the messages stream.

Without this fix, the client will receive messages by two (mpsc buffer size + 1).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

max held this revision as a draft.
max edited the test plan for this revision. (Show Details)
max added reviewers: varun, bartek.
max published this revision for review.Feb 23 2023, 4:16 AM

That's an interesting discovery! Good job on finding that!

This revision is now accepted and ready to land.Feb 24 2023, 1:59 PM
varun requested changes to this revision.Feb 24 2023, 2:00 PM
varun added inline comments.
services/tunnelbroker/src/server/mod.rs
265 ↗(On Diff #22973)

do we really want to panic here?

This revision now requires changes to proceed.Feb 24 2023, 2:00 PM
max added inline comments.
services/tunnelbroker/src/server/mod.rs
265 ↗(On Diff #22973)

do we really want to panic here?

Yes, because if we have an error here something really goes wrong. For example, an error in blocking read on the folly concurrent hashmap.

max marked an inline comment as done.
This revision is now accepted and ready to land.Feb 27 2023, 4:13 AM