Improve RequestsFetcher cleanup mechanisms

[sechkova]

Description or feature request:
Implement RequestsFetcher as a context manager and/or provide public cleanup method to be called by its users (for ex. fetcher.close() )

Current behavior:
RequestsFetcher internlly tracks all exit paths to release allocated resources which is error prone.

Expected behavior:
Reliably release all resources allocated by RequestsFetcher.

[jku]

I think fetch() being a CM might make sense (if it makes the client code nicer) but I don't believe it makes it any easier to handle cleanup...

The only resource (that requires handling) in RequestsFetcher is the requests.Response: this is tricky because we return a generator function that owns the Response but otherwise I think there's nothing that requires attention.

fetcher.close() could not solve any issues I believe: there is no object level "context" in fetcher itself that is problematic. fetch() does have to handle the Response but that can't be fixed with more methods as once we return the generator that owns the Response, it is no longer something the fetcher should or can worry about.

[sechkova]

Reviving this issue after having a look at RequestsFetcher after a while.
Implementing fetch() as a context manager makes the default TUF implementation look better because now response is closed in a single place. For example:

requests_fetcher.py:

    @contextmanager
    def fetch(self, url, required_length):
        ...
        try:
            response.raise_for_status()
            yield self._chunks(response, required_length)

        except requests.HTTPError as e:
            status = e.response.status_code
            raise exceptions.FetcherHTTPError(str(e), status)

        finally:
            response.close()

download.py:

-            chunks = self._fetcher.fetch(url, required_length)
-            for chunk in chunks:
-                temp_file.write(chunk)
-                number_of_bytes_received += len(chunk)
+            with self._fetcher.fetch(url, required_length) as chunks:
+                for chunk in chunks:
+                    temp_file.write(chunk)
+                    number_of_bytes_received += len(chunk)
+

But this also means adding a requirement to FetcherInterface.fetch() to be a context manager and I can't decide if it is adding enough value?

[jku]

@sechkova do you think this is still an active issue -- I think moving the download methods into Fetcher have solved the issues around this? I don't see our code having trouble with unclosed things currently

[sechkova]

Yeah, with download_file() being context manager and RequestsFetcher correctly cleaning up sockets, I think this is solved. Closing it.