Digital-Sapphire / PyUpdater

Pyinstaller auto-update library
https://www.pyupdater.org
460 stars 92 forks source link

Honor HTTP_TIMEOUT in FileDownloader #218

Closed mcsimps2 closed 4 years ago

mcsimps2 commented 5 years ago

Problem: Developers can set the HTTP_TIMEOUT in their client configuration. This variable does not seem to be honored in the codebase. As a consequence, there are instances where this can cause an application to hang indefinitely waiting for a response, leaving the application in a "paused" state.

Real world example: I had set the HTTP_TIMEOUT of my application to be 30 seconds. Unfortunately, my API server (which is polled in the UPDATE_URLS variable) encountered issues and started hanging incoming requests. Here's a screenshot using the CurrPorts application for Windows that shows a TCP connection, which had been hanging for many hours:

Screen Shot 2019-09-26 at 10 28 04 AM

Essentially, I had an updater thread that would poll for an update, but the whole thread hung indefinitely since the app_update.refresh() was not enforcing the HTTP_TIMEOUT setting. As a result, my application could no longer update itself in this hung state. When I manually closed the TCP connection through CurrPorts, all normal functionality resumed. My setup was pretty vanilla with just patching disabled.

Proposed Solution: Since this is my first time diving into the PyUpdater codebase, please excuse any mistakes I make during my analysis.

Doing a rough search through for the variable http_timeout in the codebase doesn't return many results. I believe we may not be honoring it as we should be.

I do not see any references again to the http_timeout variable in the FileDownloader class. For example, in the method _create_response(), the code is

data = self.http_pool.urlopen(
                    "GET", file_url, preload_content=False, retries=max_download_retries
                )

If I understand correctly, we should be passing in the timeout to this function call so that it looks more like

data = self.http_pool.urlopen(
                    "GET", file_url, preload_content=False, retries=max_download_retries, timeout=<timeout>
                )

This also means we must start passing the timeout down to FileDownloader, which we are currently not doing. For example, the LibUpdate class doesn't seem to be passing self.http_timeout to FileDownloader or even referencing the variable at all.

In other methods, such as _get_manifest_from_http() and _get_key_data(), we also don't pass the timeout down to FileDownloader - not that it makes use of it in the first place, anyways.

fd = FileDownloader(
                        vf,
                        self.update_urls,
                        verify=self.verify,
                        urllb3_headers=self.urllib3_headers,
                    )

Please let me know if I have misinterpreted any parts of the codebase, I look forward to hearing your thoughts.

JMSwag commented 4 years ago

@mcsimps2 Great catch. I’ll get this fixed in the next release.

mcsimps2 commented 4 years ago

I went ahead and addressed this in a PR I’m working on right now for #219 - I’ll tag this timeout issue in the PR too sometime this upcoming week when I push!

JMSwag commented 4 years ago

@mcsimps2 Thank you sir!