Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to 1.11.x: Add timeout to Client RPC calls (#11500) #14498

Merged
merged 2 commits into from
Sep 13, 2022

Conversation

dhiaayachi
Copy link
Collaborator

@dhiaayachi dhiaayachi commented Sep 6, 2022

Description

Adds a timeout (deadline) to client RPC calls, so that streams will no longer hang indefinitely in unstable network conditions.

Co-authored-by: kisunji ckim@hashicorp.com

Testing & Reproduction steps

  • Tests were introduced in the initial PR

Links

Port of:

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

Adds a timeout (deadline) to client RPC calls, so that streams will no longer hang indefinitely in unstable network conditions.

Co-authored-by: kisunji <ckim@hashicorp.com>
@github-actions github-actions bot added the pr/dependencies PR specifically updates dependencies of project label Sep 6, 2022
@dhiaayachi dhiaayachi added the pr/no-changelog PR does not need a corresponding .changelog entry label Sep 6, 2022
The timeout should include the maximum possible
jitter since the server will randomly add to it's
timeout a jitter. If the server's timeout is less
than the client's timeout then the client will
return an i/o deadline reached error.

Before:
```
time curl 'http://localhost:8500/v1/catalog/service/service?dc=other-dc&stale=&wait=600s&index=15820644'
rpc error making call: i/o deadline reached
real    10m11.469s
user    0m0.018s
sys     0m0.023s
```

After:
```
time curl 'http://localhost:8500/v1/catalog/service/service?dc=other-dc&stale=&wait=600s&index=15820644'
[...]
real    10m35.835s
user    0m0.021s
sys     0m0.021s
```
@dhiaayachi dhiaayachi requested review from a team, kyhavlov and kisunji and removed request for a team September 6, 2022 18:11
@dhiaayachi dhiaayachi merged commit e2dca50 into release/1.11.x Sep 13, 2022
@dhiaayachi dhiaayachi deleted the port_RPC_client_timeout branch September 13, 2022 12:57
@jkirschner-hashicorp
Copy link
Contributor

@dhiaayachi , @kisunji : Should #13344 have also been included in this backport to 1.11? I know that's come up in the context of #11500 before.

@kisunji
Copy link
Contributor

kisunji commented Sep 21, 2022

Oh yes, I think that may have been missed

@jkirschner-hashicorp
Copy link
Contributor

#13348 just backported to 1.12 as far as I can tell, not 1.11

@dhiaayachi
Copy link
Collaborator Author

@jkirschner-hashicorp @kisunji I'm not sure I understand the link between this (adding RPC timeout) and the fix introduced in #13344

@kisunji
Copy link
Contributor

kisunji commented Sep 22, 2022

@dhiaayachi There was an existing bug where KV.Get endpoint was being passed **structs.KeyRequest instead of *structs.KeyRequest but it didn't matter because RPC dereferences it correctly. However, while adding the RPC timeout we added an interface type assertion which **structs.KeyRequest did not fulfil, making it default to 7 seconds (making blocking queries fail after 7s).

@dhiaayachi
Copy link
Collaborator Author

Thank you @kisunji, I get it now 🙂
I will try to backport that, it will go in the next patch release.

@dhiaayachi
Copy link
Collaborator Author

Done: c42dde9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/dependencies PR specifically updates dependencies of project pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants