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.

Sign up for GitHub

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

Jump to bottom

Speed up source deletion #1386

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Speed up source deletion #1386

merged 3 commits into from
Jan 7, 2022

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Dec 22, 2021 •
edited
Loading

Description

Fixes #1344
Fixes #1343

Test Plan

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

If these changes modify the database schema, you should include a database migration. Please check as applicable:

  • I have written a migration and upgraded a test database based on main and confirmed that the migration applies cleanly
  • I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
  • I need help writing a database migration
  • No database schema changes are needed

@sssoleileraaa sssoleileraaa requested a review from a team as a code owner December 22, 2021 01:42
gonzalo-bulnes
gonzalo-bulnes reviewed Dec 22, 2021
View reviewed changes
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me, but I'm not confident I understand the sync-ing behavior well enough to give a full green tick. : )

securedrop_client/gui/widgets.py Outdated Show resolved Hide resolved
tests/gui/test_widgets.py Outdated Show resolved Hide resolved
tests/gui/test_widgets.py Outdated Show resolved Hide resolved
tests/test_sync.py Show resolved Hide resolved
@sssoleileraaa
Copy link
Contributor Author

I was going to add the same timestamp logic as used for conversation deletion, but i don't think we need it. Calling on @eloquence to try to break i! If it's good as is, I'll remove the timestamp from the new deletion signal. If the "ghost" issue happens, I'll add logic using the timestamp.

@sssoleileraaa
Copy link
Contributor Author

@gonzalo-bulnes feel free to try to break this too plz X). I didn't see your review until after I posted that, but always appreciate the way erik finds user-facing issues in my code!

@sssoleileraaa sssoleileraaa force-pushed the speed-up-source-deletion branch from 1b0c392 to 93ca170 Compare December 22, 2021 23:27
@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Dec 22, 2021 •
edited
Loading

Revert "Introduce immediate sync upon source/conversation deletion"

💡 So this means using a single-shot timer trigger is a known-good solution, am I correct?

@eloquence
Copy link
Member

I've taken this for a spin today and overall it looks great! Deletion is indeed much faster, and I'm not able to reproduce #1343 any longer.

I do see an issue with sources sometimes re-appearing until the next sync (similar to #858). These "ghost sources" can be interacted with, but if you attempt to reply to them, you get a "Failed to send" because they've already been deleted on the sever. The error is logged as "bad request" on the client.

The way I'm able to reproduce it:

  1. Submit a bunch of sources (can be just messages)
  2. Delete all of them in quick succession in the client
  3. Observe the source list carefully

What happens is that sometimes a source like curved jerky (actual name I got today :P) will be removed, then briefly reappear in the source list, then be removed after the next sync completes successfully.

@sssoleileraaa
Copy link
Contributor Author

The way I'm able to reproduce it:

  1. Submit a bunch of sources (can be just messages)
  2. Delete all of them in quick succession in the client
  3. Observe the source list carefully

Yup, confirmed. I think I know how to fix... I'll keep the fix in a separate commit

@eloquence
Copy link
Member

Taking this for another quick spin now.

@eloquence
Copy link
Member

Functionally all looks great to me:

conorsch pushed a commit that referenced this pull request Jan 7, 2022
Remove debugging print statement
3913be6 
Discovered while testing #1386; looks like it snuck in during #1377. Was
emitting harmless "OK" lines to stdout, never landed in the app logs on
disk. Removing now as a cleanup prior to upcoming 0.5.2 release.
conorsch
conorsch previously approved these changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Works great! The removal is snappy and complete. Tested several times against an onion service, and it was pleasant responsive. Also tested a pathological edge where the server doesn't reply with a 200, and the client appropriately raised the error and the source was not removed from the UI prematurely:

failed-to-delete-at-server

If the server did actually delete the source in question, despite responding not-ok, then the subsequent sync dropped the source from the UI, which is a nice response. When I edited the server code to respond not-ok and preserve the source in the server db, subsequent client syncs did not remove the source from the UI—also good!

I tacked on one small cleanup commit. LGTM.

@conorsch conorsch force-pushed the speed-up-source-deletion branch from 3913be6 to f8010cc Compare January 7, 2022 02:17
@conorsch
Copy link
Contributor

conorsch commented Jan 7, 2022 •
edited
Loading

I tacked on one small cleanup commit.

Ah, was already handled in d9e5546 ; just needed a rebase here.

@conorsch conorsch self-requested a review January 7, 2022 02:26
@conorsch conorsch merged commit 0a9d2b1 into main Jan 7, 2022
@conorsch conorsch deleted the speed-up-source-deletion branch January 7, 2022 02:27
@sssoleileraaa sssoleileraaa mentioned this pull request Jan 26, 2022
Closed
26 tasks
@eloquence eloquence mentioned this pull request Feb 10, 2022
Closed
@sssoleileraaa sssoleileraaa mentioned this pull request Feb 14, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gonzalo-bulnes gonzalo-bulnes gonzalo-bulnes left review comments

@conorsch conorsch conorsch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@sssoleileraaa @gonzalo-bulnes @eloquence @conorsch