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

Fix potential crash situation WRT queue. #744

Merged
merged 2 commits into from
Jan 29, 2020

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Jan 29, 2020

Description

Found while fixing #716. Perhaps addresses #662?

If the queue has an upper limit (like the metadata queue does), then the re_add_job method may try to do something that results in a queue.Full exception being thrown. This can be safely ignored. Because this case wasn't handled, I encountered an application crash while investigating #716 which led to this fix.

In addition I noticed there being a possibility of jobs being put onto the metadata queue if the user wasn't logged in (something which shouldn't happen). This has also been fixed.

Test Plan

Unit tests updated to reflect both changes.

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, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

@sssoleileraaa
Copy link
Contributor

This doesn't fix #662 (https://user-images.githubusercontent.com/213636/70762572-d5f70880-1d05-11ea-97c0-f7e2c9b086a5.png) because we will still update the error bar in the client to say we can't connect to the server when the api token is None. The fix for #662 is in this branch: when-token-invalid-user-must-log-back-in which will be a PR once adequate tests have been added and the bug is confirmed to be fixed. If you're not convinced, you can easily test whether or not your changes fix this issue by logging out when there are items left in the queue.

@sssoleileraaa
Copy link
Contributor

If you're not convinced, you can easily test whether or not your changes fix this issue by logging out when there are items left in the queue.

(I'm not entirely convinced ;))

Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

lgtm

@rmol rmol merged commit 4a7156f into freedomofpress:master Jan 29, 2020
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.

3 participants