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.