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

no longer mark replies as failed if they time out #819

Merged
merged 1 commit into from
Feb 27, 2020

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Feb 25, 2020

Description

Fixes #604
Towards #653

We have another draft PR (#818) that I'll be looking at today that is working to fix #653. This PR fixes the out-of-order issue for replies that time out (since we no longer mark them as failed). #818 will fix the out-of-order issue for replies that fail for some other reason.

This will go from draft->ready for review once I update tests.

Test Plan

Run through STR#1 for #653 and make sure replies are no longer out of order and that replies that time out are still shown as pending.

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

@sssoleileraaa sssoleileraaa force-pushed the replies-that-timeout-are-no-longer-failed branch 2 times, most recently from ef0a5bf to eff9bdd Compare February 25, 2020 21:41
@sssoleileraaa sssoleileraaa marked this pull request as ready for review February 25, 2020 21:41
@sssoleileraaa
Copy link
Contributor Author

This fixes the issue reported in #653, however I'm leaving it open and will add a new STR for non-timeout errors in that issue so that it is a two-part issue to solve.

@sssoleileraaa
Copy link
Contributor Author

This is blocked until #820 is fixed so we can properly test request timeout errors along with connection errors.

@sssoleileraaa sssoleileraaa force-pushed the replies-that-timeout-are-no-longer-failed branch from eff9bdd to 51232ac Compare February 27, 2020 23:47
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.

lgtm!

@redshiftzero redshiftzero merged commit acf130b into master Feb 27, 2020
@redshiftzero redshiftzero deleted the replies-that-timeout-are-no-longer-failed branch February 27, 2020 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replies are sometimes displayed out of order Show replies that have timed out as pending
3 participants