torbrowser-launcher
torbrowser-launcher copied to clipboard
Add connection and read timeouts to requests to prevent app hangs.
Issue
We currently make two requests that have no timeout. These could potentially block the application forever if there are network or server issues. The requests
library recommends all production code utilize timeouts for this reason.
Proposed Solution
I add a five second timeout to the two requests in common.py
and launcher.py
as well as a custom HTTP adapter that retries failed requests up to three times.
Implementation Details
I implement a custom HTTPAdapter
which configures a max of three retries in the event of a connection issue. Since this retry strategy has to be implemented via an HTTPAdapter
I also had to implement a requests.Session()
attribute on the two classes in question torbrowser_launcher.common.Common
and torbrowser_launcher.launcher.DownloadThread
, then mount that adapter to the Session()
. This session
is then used to make the GET request.
Also, instead of the if/else logic checking for a 200 status code on the response, I utilize the raise_for_status()
method on the response object. If the response is not a 200 OK, this will raise a requests.HTTPError
. I then move the error handling logic intended for that situation into a handler for that exception where, I would argue, it belongs.
Exception Handling
In the event of a timeout, the requests
library will raise either a ConnectTimeout
or a ReadTimeout
, both of which are subclasses of Timeout
and in the case of ConnectTimeout
it is also subclasses ConnectionError
(see linked docs).
In the case of the download timing out in launcher.py
, if it was a ConnectTimeout
the existing error handling for ConnectionError
will execute. In the event it was a ReadTimeout
an unhandled exception would occur.
An open question is if we want a ReadTimeout
to be unhandled or if we should gracefully handle a ReadTimeout
differently or perhaps even just have the handler for ConnectionError
also handle ReadTimeout
? The existing error language for a ConnectionError
here works well in the event there was a ConnectTimeout
. It is less accurate if there was a ReadTimeout
e.g. we made a connection successfully but the server failed to send us data in the specified timeframe. In such a case we probably don't want to say "Error starting download..." which is why I hesitate to just handle ConnectionError
and Timeout
together here. Maybe we can do that and just edit the "starting" language to be something more general like "Error while downloading"?
In common.Common
I just follow the existing strategy of printing an error to the console if there's a request exception. In this case, I catch either type of timeout by catching requests.Timeout
.
Please review the Exception Handling section of this PR and provide any feedback on the open questions in there. Thanks!