Make sync continuous #739

sssoleileraaa · 2020-01-28T02:38:17Z

Description

Followup from #652
Fixes #672
Fixes #671

This is the last PR for the sync architecture change to have a continuous metadata syncs that occur frequently and unpause the queue when they succeed

This is dependent on the following PRs getting merged first (for now I cherry-picked them here so others working on the client will have visibility):

remove sync_api when files are missing for opening, exporting, or printing a file: update gui instead of sync when file missing #724
removing sync_api when a reply is sent: no longer sync after sending a reply #722

This PR includes the following changes:

removing the metadata queue in favor of a background thread that doesn't pause when other queues are paused
removing the last sync_api call on login and removing sync_api all together

Test Plan

See that the GUI still shows the active sync animation in the top left of the client
Force the queues to pause and see how our background sync thread will continue to run syncs until one succeeds and unpauses the queues

sssoleileraaa · 2020-01-28T02:39:26Z

This cannot be merged until #732 is merged, which removes user-initiated syncs via the Refresh button in the upper left-hand corner of the client.

sssoleileraaa · 2020-01-28T21:27:22Z

securedrop_client/logic.py

@@ -334,7 +337,7 @@ def on_authenticate_success(self, result):
        self.gui.show_main_window(user)
        self.update_sources()
        self.api_job_queue.login(self.api)
-        self.sync_api()


this is where we used to call sync_api on login which i replaced with starting the background sync thread

sssoleileraaa · 2020-01-28T21:28:20Z

securedrop_client/logic.py

@@ -344,6 +347,7 @@ def on_authenticate_failure(self, result: Exception) -> None:
        error = _('There was a problem signing in. '
                  'Please verify your credentials and try again.')
        self.gui.show_login_error(error=error)
+        self.api_sync.stop()


when we log out we need to make sure the background sync thread does not continue making api requests to get new data from the server

securedrop_client/logic.py

securedrop_client/queue.py

sssoleileraaa · 2020-01-28T21:31:40Z

I left some inline comments to help with review. ~~This is ready.~~ Update: Actually it makes sense that this should be blocked until the current sync_api removal prs are merged.

sssoleileraaa · 2020-01-30T21:39:56Z

This is ready for review. The last PR dependency this relies on is this one: #737 (removal of sync_api from the gui and controller when file is missing).

For now you can cherry-pick the 2 commits from that PR to here to see tests pass and review the code.

sssoleileraaa · 2020-01-31T21:38:02Z

This is no longer blocked

sssoleileraaa · 2020-02-03T23:47:42Z

rebased on master branch and resolved conflicts with all the great changes coming in

redshiftzero · 2020-02-04T02:47:58Z

I started looking at this today - this is looking pretty good to me but I'll finish review tomorrow AM and drop any comments in the PR for when you sign on @creviera

securedrop_client/sync.py

sssoleileraaa · 2020-02-05T04:07:00Z

review comments addressed

sssoleileraaa · 2020-02-05T04:08:57Z

rebased on master branch and signed commits

redshiftzero · 2020-02-06T01:00:14Z

excellent, my comments above were addressed, thanks! so there's an unexpected behavior that I observed when I was running this in Qubes: when I paused my app-staging VM in order to test the pause behavior, I got the login screen and a "session expired, login again" [paraphrasing] message - have you seen this too? My expectation was that I'd see a pause and the retry banner... I'm not sure if it's related to this change yet

sssoleileraaa · 2020-02-06T02:05:35Z

when I paused my app-staging VM in order to test "session expired, login again" [paraphrasing] message - have you seen this too?

I have not seen this but I'll work on repro'ing. This would mean that the MetadataSyncJob is seeing an ApiInaccessibleError when this occurs, so I'll pay close attention to the logs, and will also check master. I believe we're correctly interpreting AuthError to mean the token is invalid/expired, so my guess is that the local api_client is getting set to None somewhere when it shouldn't be.

Update:

We are correctly interpreting AuthError to mean the token is invalid/expired but the SDK is incorrectly sending request timeout and other types of errors as AuthErrors. See comment below for explanation.

sssoleileraaa · 2020-02-06T04:28:35Z

It looks like this is occurring because of this issue I just opened up in the SDK repo: freedomofpress/securedrop-sdk#109

The reason this has gone unnoticed on master is because we don't ask the user to provide credentials when we see an ApiInaccessibleError. Instead we just pause the queue the way we pause the queue for request timeouts as well.

Once the client begins receiving RequestTimeoutError instead of an AuthError this will work. I also wrote up in that sdk issue that it would be valuable to be able to receive a new ConnectTimeout error to indicate that we can't reach the server as opposed to a request timeout or read timeout.

kushaldas · 2020-02-06T10:17:49Z

While running the branch, I am getting the following error in the log:

2020-02-06 15:46:12,722 - securedrop_client.queue:145(process) ERROR: Skipping job
2020-02-06 15:46:12,803 - securedrop_client.api_jobs.downloads:149(_download) INFO: File downloaded to /tmp/tmp.lkSEe1s2av/data/colorless_throes/1-colorless_throes-msg.txt
2020-02-06 15:46:12,910 - securedrop_client.api_jobs.downloads:168(_decrypt) INFO: File decrypted: 1-colorless_throes-msg.txt (decrypted file: )
2020-02-06 15:46:12,912 - securedrop_client.queue:144(process) ERROR: Job <securedrop_client.queue.RunnableQueue object at 0x7fdca48094c8> raised an exception: AttributeError: 'RunnableQueue' object has no attribute 'pinged'
2020-02-06 15:46:12,913 - securedrop_client.queue:145(process) ERROR: Skipping job
2020-02-06 15:46:13,019 - securedrop_client.api_jobs.downloads:149(_download) INFO: File downloaded to /tmp/tmp.lkSEe1s2av/data/colorless_throes/2-colorless_throes-msg.txt
2020-02-06 15:46:13,177 - securedrop_client.api_jobs.downloads:168(_decrypt) INFO: File decrypted: 2-colorless_throes-msg.txt (decrypted file: )
2020-02-06 15:46:13,179 - securedrop_client.queue:144(process) ERROR: Job <securedrop_client.queue.RunnableQueue object at 0x7fdca48094c8> raised an exception: AttributeError: 'RunnableQueue' object has no attribute 'pinged'
2020-02-06 15:46:13,179 - securedrop_client.queue:145(process) ERROR: Skipping job
2020-02-06 15:46:13,286 - securedrop_client.api_jobs.downloads:149(_download) INFO: File downloaded to /tmp/tmp.lkSEe1s2av/data/prepositional_technicolor/1-prepositional_technicolor-msg.txt

sssoleileraaa · 2020-02-06T17:11:42Z

ah looks like a pinged signal was missed in the removal

redshiftzero

all my comments were addressed, and kushal's comment was addressed in the latest change so I'm gonna approve this 🚢

redshiftzero · 2020-02-06T21:14:27Z

there is still the outstanding issue discovered during review here, but that will be resolved separately as part of freedomofpress/securedrop-sdk#109

sssoleileraaa added the blocked label Jan 28, 2020

sssoleileraaa force-pushed the make-sync-continuous branch 2 times, most recently from 0a8be5e to 9afaed8 Compare January 28, 2020 02:47

sssoleileraaa marked this pull request as ready for review January 28, 2020 05:22

sssoleileraaa requested review from kushaldas and redshiftzero as code owners January 28, 2020 05:22

eloquence added this to In Development in SecureDrop Team Board Jan 28, 2020

This was referenced Jan 28, 2020

Ensure UI remains responsive at all times during sync with server. #733

Merged

no longer sync after sending a reply #722

Merged

sssoleileraaa force-pushed the make-sync-continuous branch from 9e7d342 to 3e1c31f Compare January 28, 2020 21:25

sssoleileraaa removed the blocked label Jan 28, 2020

sssoleileraaa commented Jan 28, 2020

View reviewed changes

securedrop_client/logic.py Outdated Show resolved Hide resolved

sssoleileraaa commented Jan 28, 2020

View reviewed changes

securedrop_client/queue.py Show resolved Hide resolved

sssoleileraaa added the blocked label Jan 29, 2020

eloquence mentioned this pull request Jan 29, 2020

"Last Refresh:" should only show up if user is offline or experiencing network issues #747

Open

sssoleileraaa force-pushed the make-sync-continuous branch from 5323149 to 8021f5b Compare January 30, 2020 21:37

sssoleileraaa moved this from In Development to Ready for Review in SecureDrop Team Board Jan 30, 2020

sssoleileraaa force-pushed the make-sync-continuous branch 2 times, most recently from 560a53d to 11c09c6 Compare January 31, 2020 21:33

sssoleileraaa removed the blocked label Jan 31, 2020

sssoleileraaa force-pushed the make-sync-continuous branch from 7b827c8 to d2c49eb Compare February 3, 2020 23:47

redshiftzero moved this from Ready for Review to Under Review in SecureDrop Team Board Feb 4, 2020

redshiftzero suggested changes Feb 4, 2020

View reviewed changes

securedrop_client/sync.py Outdated Show resolved Hide resolved

securedrop_client/sync.py Outdated Show resolved Hide resolved

eloquence added the release blocker label Feb 4, 2020

sssoleileraaa force-pushed the make-sync-continuous branch from eaa3a54 to 20050ba Compare February 5, 2020 04:07

Allie Crevier added 8 commits February 4, 2020 20:08

create ApiSync class that continuously syncs

9efe65a

update and add tests for new background sync

0805b97

make sync handlers private

410cc21

stop metadata sync thread when user logs out

3faed87

remove sync_api call and metadata sync queue

1292065

update tests after metadata queue and sync_api removal

39d13fc

resolve conflicts

3cfab75

add api sync background task

417473b

sssoleileraaa force-pushed the make-sync-continuous branch from 20050ba to 417473b Compare February 5, 2020 04:08

change number of timeout retries for a sync to 2

bcfc9c5

redshiftzero mentioned this pull request Feb 6, 2020

pure refactor: move auth checks into decorator #780

Merged

6 tasks

remove emitting pinged signal

94b1721

sssoleileraaa requested a review from redshiftzero February 6, 2020 17:15

redshiftzero approved these changes Feb 6, 2020

View reviewed changes

redshiftzero merged commit cdb1263 into master Feb 6, 2020

SecureDrop Team Board automation moved this from Under Review to Done Feb 6, 2020

redshiftzero deleted the make-sync-continuous branch February 6, 2020 21:15

This was referenced Feb 7, 2020

fix 109, rework exception handling freedomofpress/securedrop-sdk#110

Merged

app: use ServerConnectionError from SDK 0.0.13 (resolves failed sync causing AuthError, and KeyError during a failed download) #784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sync continuous #739

Make sync continuous #739

sssoleileraaa commented Jan 28, 2020 •

edited

sssoleileraaa commented Jan 28, 2020

sssoleileraaa Jan 28, 2020

sssoleileraaa Jan 28, 2020

sssoleileraaa commented Jan 28, 2020 •

edited

sssoleileraaa commented Jan 30, 2020

sssoleileraaa commented Jan 31, 2020

sssoleileraaa commented Feb 3, 2020

redshiftzero commented Feb 4, 2020

sssoleileraaa commented Feb 5, 2020

sssoleileraaa commented Feb 5, 2020

redshiftzero commented Feb 6, 2020

sssoleileraaa commented Feb 6, 2020 •

edited

sssoleileraaa commented Feb 6, 2020

kushaldas commented Feb 6, 2020

sssoleileraaa commented Feb 6, 2020

redshiftzero left a comment

redshiftzero commented Feb 6, 2020

Make sync continuous #739

Make sync continuous #739

Conversation

sssoleileraaa commented Jan 28, 2020 • edited

Description

Test Plan

sssoleileraaa commented Jan 28, 2020

sssoleileraaa Jan 28, 2020

Choose a reason for hiding this comment

sssoleileraaa Jan 28, 2020

Choose a reason for hiding this comment

sssoleileraaa commented Jan 28, 2020 • edited

sssoleileraaa commented Jan 30, 2020

sssoleileraaa commented Jan 31, 2020

sssoleileraaa commented Feb 3, 2020

redshiftzero commented Feb 4, 2020

sssoleileraaa commented Feb 5, 2020

sssoleileraaa commented Feb 5, 2020

redshiftzero commented Feb 6, 2020

sssoleileraaa commented Feb 6, 2020 • edited

sssoleileraaa commented Feb 6, 2020

kushaldas commented Feb 6, 2020

sssoleileraaa commented Feb 6, 2020

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero commented Feb 6, 2020

sssoleileraaa commented Jan 28, 2020 •

edited

sssoleileraaa commented Jan 28, 2020 •

edited

sssoleileraaa commented Feb 6, 2020 •

edited