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

move metadata sync to api queue #640

Merged
merged 3 commits into from
Dec 5, 2019
Merged

move metadata sync to api queue #640

merged 3 commits into from
Dec 5, 2019

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented Dec 5, 2019

Description

Fixes #461
Fixes #489 (between this change and the pending reply change we should not see that bug any more)

This PR implements the MetadataSyncJob. A couple notes:

  • I left in on_sync_failure which is what is creating the "The SecureDrop server cannot be reached" messages so we can observe over the coming days/weeks how often this is failing (if we still see failures, we can bump up the number of automatic tries in the queue)
  • I hit an upstream cpython bug where a line is incorrectly showing as uncovered, see full explanation in and relevant links in e037080

Test Plan

  1. Open client
  2. Check client populates with messages/replies/etc.
  3. Send a message as a new source
  4. Click sync, check the new message appears

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 (just moves things around, doesn't change any qubes-specific logic)

This line is definitely being executed by
test_MetadataSyncJob_success_with_missing_key
but curiously it wasn't being shown as executed in our coverage
report.

Investigating, I discovered the following upstream bug:
https://bugs.python.org/issue2506

The cpython peephole optimizer is causing this line to report
as uncovered (here's a nice blog explaining this optimizer:
https://akaptur.com/blog/2014/08/02/the-cpython-peephole-optimizer-and-you/
this helpfully uses an example (see the section under "Complications arise"
where code is reported as uncovered due to peephole optimizations))
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

This is a very clean migration. I just need to test on Qubes, but looking good so far!

@sssoleileraaa sssoleileraaa self-requested a review December 5, 2019 19:03
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

I ran through the normal manual regression testing procedure and found three regressions:

  • staring does not immediately sync (it's much slower now)
  • ^ same for deleting a source + the UI does not immediately update: you click delete and it just stays there for 30 seconds
  • The refresh icon no longer shows that it's syncing during a delete or staring operation. Particularly for delete, I think we should see something happening in the background because it looks like now it just doesn't work for 30 seconds

@sssoleileraaa
Copy link
Contributor

  • staring does not immediately sync (it's much slower now)

Looks like staring is not slower afterall so this is safe to ignore

  • ^ same for deleting a source + the UI does not immediately update: you click delete and it just stays there for 30 seconds

This is an actual issue since the UI used to update immediately to reflect the deletion

  • The refresh icon no longer shows that it's syncing during a delete or staring operation. Particularly for delete, I think we should see something happening in the background because it looks like now it just doesn't work for 30 seconds

This can be ignored. For some reason I thought the refresh icon would go into an active state during a deletion, but that shouldn't be the case and never was.

@sssoleileraaa sssoleileraaa self-requested a review December 5, 2019 23:30
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

This is an actual issue since the UI used to update immediately to reflect the deletion

Looks like this is also an issue on master after more testing. We could continue the conversation of it being slow in this issue or followup issue: #462 (comment)

Also, I created an issue for showing that a deletion is in progress in the UI in case we don't show removal right away clientside: #643

@sssoleileraaa sssoleileraaa merged commit b7c2839 into master Dec 5, 2019
@sssoleileraaa sssoleileraaa deleted the metadata-sync-job branch December 5, 2019 23:37
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.

replies.file_counter unique constraint fails and crashes app Create a MetadataSyncJob
2 participants