[services] Tunnelbroker - Adding the gRPC server keep-alive into the Tonic server
ClosedPublic
Actions

Authored by • max on Nov 23 2022, 9:25 AM.

Details

Reviewers

Commits

Summary

This diff introduces removing the "pinging" async task from the gRPC server and using the gRPC http/2 keep-alive support of the Tonic.
The full context is located in the ENG-1842 task. Research and proof-of-work of the gRPC keep-alive using Tonic.

Related Linear task: ENG-1842

Test Plan

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

• max held this revision as a draft.

• max published this revision for review.Nov 23 2022, 9:30 AM

• max edited the summary of this revision. (Show Details)

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

• max added reviewers: marcin, varun. • max added 1 blocking reviewer(s): • jon.

• max edited the summary of this revision. (Show Details)

Overall looks good

services/tunnelbroker/src/server/mod.rs
394 ↗	(On Diff #18789)	to avoid the `.into()` call, we could do `constants::GRPC_KEEP_ALIVE_PING_INTERVAL_SEC as u64` or just declare the constants as u64 https://doc.rust-lang.org/std/time/struct.Duration.html#method.from_secs

This revision is now accepted and ready to land.Nov 28 2022, 8:47 AM

Switching to use u64 instead of converting from u8 by the into().

services/tunnelbroker/src/server/mod.rs
394 ↗	(On Diff #18789)	to avoid the `.into()` call, we could do `constants::GRPC_KEEP_ALIVE_PING_INTERVAL_SEC as u64` or just declare the constants as u64 https://doc.rust-lang.org/std/time/struct.Duration.html#method.from_secs Yes, makes sense to just use an `u64`. Seems that we will not have any memory overhead using the bigger integer type because Rust constants are just inlined at the compile time and will not be associated with any memory. Thanks, @jon !

This revision now requires review to proceed.Nov 29 2022, 11:39 AM

might be worth explaining in constants.rs the rationale behind the chosen interval and timeout times

tomek added inline comments.

services/tunnelbroker/src/constants.rs
3–4 ↗	(On Diff #18973)	How about storing `Duration` directly? By doing that we can avoid stating a unit in the name.
services/tunnelbroker/src/server/mod.rs
353–358 ↗	(On Diff #18973)	How this corresponds to `updateSessionItemIsOnline` function we have? Is it going to be called when a connection times out?

This revision is now accepted and ready to land.Nov 30 2022, 5:16 AM

Switch to use Duration in constants instead of uint.

In D5713#171992, @varun wrote:

might be worth explaining in constants.rs the rationale behind the chosen interval and timeout times

Honestly, not sure that we should add this because intervals and timeouts were part of the research including some field tests. I don't think it should be here 🤷‍♂️

services/tunnelbroker/src/constants.rs
3–4 ↗	(On Diff #18973)	How about storing `Duration` directly? By doing that we can avoid stating a unit in the name. That's a good idea and a more efficient way to store it! Thanks @tomek ! I've made these changes.
services/tunnelbroker/src/server/mod.rs
353–358 ↗	(On Diff #18973)	How this corresponds to `updateSessionItemIsOnline` function we have? Is it going to be called when a connection times out? Yes, it's called when the stream ended (by the ping timeout or the client closed it) here and on fail to write to the stream here. A simple test showed that the keep-alive works as expected in a test repo.