SendReplyJobTimeoutError now derived from Exception #824
Conversation
sssoleileraaa
force-pushed
the
fix-issue-820
branch
from
3104bf7 to
bcb8295
Compare
sssoleileraaa
requested review from
kushaldas and
redshiftzero
as code owners
| @@ -96,7 +96,7 @@ def _make_call(self, encrypted_reply: str, api_client: API) -> sdclientapi.Reply | |||
| # TODO: Once https://github.com/freedomofpress/securedrop-client/issues/648, we will want to | |||
| # pass the default request timeout to reply_source instead of setting it on the api object | |||
| # directly. | |||
| api_client.default_request_timeout = 5 | |||
| api_client.default_request_timeout = 0.01 | |||
There was a problem hiding this comment.
accidentally left in?
| self.failure_signal.emit(e) | ||
| raise | ||
| else: | ||
| self.success_signal.emit(result) |
There was a problem hiding this comment.
the effect of removing this automatic retry logic could be that a user will see a notification much more often... I think we should understand (roughly) how often the request succeeds on the first try - if it often needs to be retried and succeeds on a subsequent try then we should keep this logic
There was a problem hiding this comment.
my thinking is that retrying 5 times (5 being the default) when we get a ServerConnectionError wouldn't result in more success. it doesn't hurt to retry 5 times back to back, but i don't think it's necessary. we could just fall back to the queue pause/resume-when-sync-can-connect-to-the-server logic. retrying 5 times when we get a RequestTimeoutError could result in more success, but i think more commonly if the request times out after X seconds, since we don't increase that time in the next attempt, it'll probably time out again and again, so it seems to make sense to pause the queue and rely on queue retry logic.
There was a problem hiding this comment.
but i think more commonly if the request times out after X seconds, since we don't increase that time in the next attempt, it'll probably time out again and again, so it seems to make sense to pause the queue and rely on queue retry logic.
@redshiftzero pointed out to me that we would end up notifying the user that we "Trying to reconnect to the SecureDrop server" after one attempt that times out, so even if it's not too common for this to occur, it would be worth researching first before removing the job retry logic and err on the side of caution so that we don't over-notify the user about network blips.
| @@ -106,7 +106,7 @@ def __init__(self, message: str, reply_uuid: str): | |||
| self.reply_uuid = reply_uuid | |||
|
|
|||
|
|
|||
| class SendReplyJobTimeoutError(RequestTimeoutError): | |||
| class SendReplyJobTimeoutError(Exception): | |||
There was a problem hiding this comment.
since the exception in #820 UnboundLocalError, I would expect that the exception type that the job should raise should be SendReplyJobError - what am I missing?
Description
Fixes #820
Unblocks #819
Unblocks #823
SendReplyJobTimeoutErrornow derives fromExceptioninstead ofsdclientapi.RequestTimeoutErrorsince it appears that deriving fromsdclientapi.RequestTimeoutErrormakes it so we can't accessself.reply_uuidfrom the Controller when we pass it the exception. I admit this is a fix for an issue I don't fully understand (it seems perfectly reasonable to me to derive fromsdclientapi.RequestTimeoutErrorwhich derives fromException, but this does fix the issue.RequestTimeoutErrororServerConnectionErrorsince we already retry jobs that fail for those reasons in the queue. Overall I think this simplifies the code, plus it makes sense not to retry a bunch of times back to back if we get aServerConnectionError. I also think our timeouts are long enough to account for when the network is temporarily slow, plus we would just pause the queue until the network is fast enough to retry the job anyway.Test Plan
Make sure #820 (comment) and #820 (comment) no longer happen
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: