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

increase sync timeout #1055

Merged
merged 1 commit into from
Apr 9, 2020
Merged

increase sync timeout #1055

merged 1 commit into from
Apr 9, 2020

Conversation

sssoleileraaa
Copy link
Contributor

Description

We are increasing the qrexec subprocess timeout for get_sources, get_all_submissions, and get_all_replies from 40 to 60 while we continue to investigate workstation performance.

I also think it would make sense to decrease the number of retries to 1 so that we can inform the user of a connection issue after 2 minutes instead of 3. I can add that to here if others agree.

Test Plan

Make sure there are no regressions.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

  • I have updated the AppArmor profile
  • No update to the AppArmor profile is required for these changes
  • I don't know and would appreciate guidance

@sssoleileraaa
Copy link
Contributor Author

We could even decrease the number of retries to zero to inform the user after one minute and since we're auto retrying anyway.

@redshiftzero
Copy link
Contributor

If we know that we're currently seeing a higher sync timeout rate, won't decreasing the number of retries increase the risk that the user will see error messages (that will get removed on a subsequent sync)?

@sssoleileraaa
Copy link
Contributor Author

If we know that we're currently seeing a higher sync timeout rate, won't decreasing the number of retries increase the risk that the user will see error messages (that will get removed on a subsequent sync)?

yes, this is true. right now we tell the user about any connection or timeout error after 2 minutes. if this pr is merged as is, we would report after 3 minutes. but this is a good point, decreasing to 1 minute before we show an error message might be too aggressively alarming, especially since we will auto recover if the issue was intermittent and related to a slow network vs the server being down or something else wrong. it would be useful to know how often these errors actually recover on their own versus a user having to restart the client or run the connection wizard. i like your idea (discussed outside of this pr) of showing the last_updated timestamp first, maybe after a sync fails to connect to the server, and then after 5 minutes or some amount of time show the error message in the error bar.

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging this now so it's in the latest nightly - thanks!

@redshiftzero redshiftzero merged commit 04a9c45 into master Apr 9, 2020
@redshiftzero redshiftzero deleted the bumpuptimeout branch April 9, 2020 23:11
@redshiftzero redshiftzero mentioned this pull request May 7, 2020
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants