chore(client): retry api version discovery for at least 30 seconds #5290
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elsirion
previously approved these changes
May 16, 2024
|
Needs rebase :( |
|
Will this fix #5286? |
I believe something else already fixed it in @joschisan's PR. But if it had been there, it would probably also prevented it. |
During debugging with joschisan, we've spotted a case where in a test the Federation was not yet fully up (peer api servers were down), yet the test Client was already attempting to join with a pre-prepared config. Normally in this case fetching the config would fail, but that was passed straight from the server fixture, so api discovery call was actually the first call. It queried all peers, all were down, the request logic would re-attempt connection but only once, which instantly failed as well, failing the whole initial api version discovery and disabling all client modules. While in this case it seems like a combination of test failing to setup Federation before attempting to run client code, it makes me think that in real life there might be cases where the client is joining new Federation, gets a config, and then e.g. switches from wifi to GSM, etc. and has a brief period of being offline. Again - this is only ever a problem on a first time client is joining. But to prevent such a hiccups, in this change, I'm making it so the client will attempt at least for a certain period to discover the version before giving it up, to lower the chances of such an unfortunate UX.
dpc
force-pushed
the
24-05-14-retry-version-discovery-longer
branch
from
d80fc13
to
784b97c
Compare
|
@dpc I did not fix this in general, just prevented this by awaiting the api to come online. |
Yeah The client can't join federation if it's not online yet. Waiting for it in the test fixture is the right fix, IMO. |
elsirion
approved these changes
May 19, 2024
Comment on lines
+1406
to
+1422
| let deadline = now() + Duration::from_secs(30); | ||
|
|
||
| loop { | ||
| let res = | ||
| Self::discover_common_api_version_static_try(config, client_module_init, api, mode) | ||
| .await; | ||
|
|
||
| if res.is_ok() { | ||
| return res; | ||
| } | ||
|
|
||
| if deadline < now() { | ||
| return res; | ||
| } | ||
| debug!(target: LOG_CLIENT, "Retrying getting api version from Federation"); | ||
| sleep(Duration::from_secs(1)).await; | ||
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During debugging with joschisan, we've spotted a case where in a test the Federation was not yet fully up (peer api servers were down), yet the test Client was already attempting to join with a pre-prepared config.
Normally in this case fetching the config would fail, but that was passed straight from the server fixture, so api discovery call was actually the first call. It queried all peers, all were down, the request logic would re-attempt connection but only once, which instantly failed as well, failing the whole initial api version discovery and disabling all client modules.
While in this case it seems like a problem of test failing to setup Federation before attempting to run client code, it makes me think that in real life there might be cases where the client is joining new Federation, gets a config, and then e.g. switches from wifi to GSM, etc. and has a brief period of being offline.
Again - this is only ever a problem on a first time client is joining. But to prevent such a hiccups, in this change, I'm making it so the client will attempt at least for a certain period to discover the version before giving it up, to lower the chances of such an unfortunate UX.