Page MenuHomePhabricator
Differential D13355

[grpc_clients] bump gRPC connection timeout to 30 seconds
ClosedPublic
Actions

Authored by varun on Sep 16 2024, 3:57 PM.
Tags
None
Referenced Files
F3340156: D13355.diff
Thu, Nov 21, 9:26 PM
Unknown Object (File)
Wed, Nov 20, 7:03 AM
Unknown Object (File)
Wed, Nov 20, 7:03 AM
Unknown Object (File)
Wed, Nov 20, 7:03 AM
Unknown Object (File)
Wed, Nov 20, 7:03 AM
Unknown Object (File)
Wed, Nov 20, 7:03 AM
Unknown Object (File)
Wed, Nov 20, 7:03 AM
Unknown Object (File)
Wed, Nov 20, 6:39 AM
View All 75 Files
Subscribers
tomek

Details

Reviewers
ashoat
bartek
will
Commits
rCOMM849adf8a969d: [grpc_clients] bump gRPC connection timeout to 30 seconds
Summary

resolves https://linear.app/comm/issue/ENG-9300/bump-identity-service-timeouts

users with bad internet can sometimes see their client time out while talking to identity. it seems that 5 seconds was too short, so bumping to 30 seconds.

Test Plan

tested this in D10329. also tested the native app with increased timeout to confirm there are no regressions

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Sep 16 2024, 3:57 PM
Herald added a subscriber: tomek. · View Herald TranscriptSep 16 2024, 3:57 PM
Harbormaster completed remote builds in B31656: Diff 44258.Sep 16 2024, 4:14 PM
varun requested review of this revision.Sep 16 2024, 4:14 PM
will accepted this revision.Sep 16 2024, 4:16 PM
This revision is now accepted and ready to land.Sep 16 2024, 4:16 PM
ashoat added a comment.Sep 16 2024, 4:51 PM

Does this affect both native and web?

I think it might be better to bump to something higher. Would there be any downside to 20s or 30s?

varun added a comment.Sep 17 2024, 7:10 AM
In D13355#376247, @ashoat wrote:

Does this affect both native and web?

I think it might be better to bump to something higher. Would there be any downside to 20s or 30s?

Just native and keyserver. The only potential downside is that 20s or 30s may feel unresponsive to users.

varun updated this revision to Diff 44274.Sep 17 2024, 8:46 AM

add a request timeout of 30 seconds

varun requested review of this revision.Sep 17 2024, 8:51 AM
varun added 1 blocking reviewer(s): ashoat.

30 seconds feels excessive for establishing the tcp connection, but i think it makes sense for completing the actual request

setting @ashoat as blocking

varun removed 1 blocking reviewer(s): ashoat.Sep 17 2024, 8:56 AM

nvm. applying @ashoat 's feedback as intended and deferring the request timeout work

varun updated this revision to Diff 44277.Sep 17 2024, 8:56 AM

undo request timeout addition

varun updated this revision to Diff 44278.Sep 17 2024, 8:58 AM
varun retitled this revision from [grpc_clients] bump gRPC connection timeout to 10 seconds to [grpc_clients] bump gRPC connection timeout to 30 seconds.
varun edited the summary of this revision. (Show Details)

fix title and description

This revision was not accepted when it landed; it landed in state Needs Review.Sep 17 2024, 8:59 AM
This revision was landed with ongoing or failed builds.
Closed by commit rCOMM849adf8a969d: [grpc_clients] bump gRPC connection timeout to 30 seconds (authored by varun). · Explain Why
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMM849adf8a969d: [grpc_clients] bump gRPC connection timeout to 30 seconds.
Harbormaster completed remote builds in B31669: Diff 44274.Sep 17 2024, 9:04 AM
Harbormaster completed remote builds in B31672: Diff 44277.Sep 17 2024, 9:15 AM
Harbormaster completed remote builds in B31673: Diff 44278.Sep 17 2024, 9:20 AM
inka mentioned this in D13750: [lib] Handle identity timeout when fetching device lists.Oct 18 2024, 3:42 AM

Revision Contents
Changeset List

PathSize
shared/
grpc_clients/
src/
2 lines

Diff 44277

View Options

shared/grpc_clients/src/lib.rs

Loading...