[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

• jon
marcin
varun
tomek

Commits

rCOMM508b86102c9a: [services] Tunnelbroker - Adding the gRPC server keep-alive into the Tonic…

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

Service is successfully built.
Tonic keep-alive "field" tests were successfully passed.

Diff Detail

Repository

rCOMM Comm

Branch

use-grpc-keep-alive

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Nov 23 2022, 9:25 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptNov 23 2022, 9:25 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript

• 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)

Harbormaster completed remote builds in B13709: Diff 18789.Nov 23 2022, 9:36 AM

• max edited the summary of this revision. (Show Details)Nov 28 2022, 4:30 AM

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 !

• max added a reviewer: tomek.Nov 29 2022, 11:39 AM

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

Harbormaster completed remote builds in B13852: Diff 18973.Nov 29 2022, 11:55 AM

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

tomek accepted this revision.Nov 30 2022, 5:16 AM

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.