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

Remove timeouts for API calls from the client and use SDK for timeouts #362

Merged
merged 6 commits into from
May 15, 2019

Conversation

heartsucker
Copy link
Contributor

@heartsucker heartsucker commented May 7, 2019

Fixes #323. Fixes #324. Fixes #325. Toward #326. Fixes #327.

This is the first pass that shows that we can easily remove the timeouts from the client. This is only toward #326 and doesn't fix it since we should add logic to extend the timeout to be even longer for very large files.

Blocked by freedomofpress/securedrop-sdk#80 (PR that adds timeouts to the SDK itself).

Testing

To test this, login. Then, go into SD core, go into journalist_app/api.py, and in setup_g add import time; time.sleep(30). Save the file. Flask will refresh. Now all requests will be delayed by 30 seconds, and the SDK timeout is 20. This will cause all requests to raise a timeout error. Check the logs for the presence of this error.

@redshiftzero
Copy link
Contributor

I haven't tested this, just reviewed the diff, but this approach seems much cleaner and simpler to me (and to the interested observer this will also resolve #325 and #323) 🙌

@heartsucker
Copy link
Contributor Author

Because of the longer timeouts in the SDK for downloads, this also gets #326. We also get #324 because we aren't using QThreads at all.

@heartsucker heartsucker force-pushed the sdk-timeouts branch 3 times, most recently from a14ea4e to 53b0696 Compare May 13, 2019 12:29
@heartsucker heartsucker force-pushed the sdk-timeouts branch 2 times, most recently from b5c007d to 953122c Compare May 14, 2019 09:19
@heartsucker heartsucker marked this pull request as ready for review May 14, 2019 09:20
@sssoleileraaa sssoleileraaa self-requested a review May 15, 2019 20:35
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.

💥 Lotta "Fixes" in this PR 💥

Note: to test this, I updated setup_g in journalist_app/__init__.py to cause a timeout and saw the expected error message in the error status bar in the client.

LGTM

@sssoleileraaa sssoleileraaa merged commit 06df84c into master May 15, 2019
@sssoleileraaa sssoleileraaa deleted the sdk-timeouts branch May 15, 2019 20:48
gonzalo-bulnes added a commit that referenced this pull request May 29, 2022
The assertions were missing when the behavior was initially added,
which explains why some of the signal name copy-pasting mistakes
were missed.

The timeout callback was removed in 45163e0.
See also #362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants