Skip to content

Conversation

@jku
Copy link
Member

@jku jku commented Jan 17, 2022

from commit description:

This commit tries to deal with two interests:
* metadata is highly repetitive and compressible: allowing compression
would be good
* there may be broken web servers (see
https://github.com/pypa/pip/blob/404838abcca467648180b358598c597b74d568c9/src/pip/_internal/download.py#L842)
that have problems with compression on already compressed target files

We can make things better for that first interest while we have no real
data for the second interest -- our current workarounds to avoid
compression are based on hearsay, not testing.

Now that individual fetchers are possible I suggest we simplify
ngclient and allow compression. As an example the pip Fetcher
could still use the pip response chunking code with all their
workarounds -- pip certainly has better capability to maintain
a mountain of workarounds and also has endless amounts of real-world
testing compared to python-tuf.

Details:
* Stop modifying Accept-Encoding (Requests default includes gzip)
* Don't use response.raw in RequestsFetcher as there is no need anymore
* Fix issue in test_session_get_timeout(): it's not mocking the error that requests really raises in this case

Fixes #1251

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@coveralls
Copy link

coveralls commented Jan 17, 2022

Pull Request Test Coverage Report for Build 1745548324

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0008%) to 97.691%

Totals Coverage Status
Change from base Build 1745219259: -0.0008%
Covered Lines: 4086
Relevant Lines: 4167

💛 - Coveralls

@kairoaraujo
Copy link
Contributor

LGTM
I'm not sure about the third commit as well.
As we talked about before, requests support it, so we are testing something outside the TUF domain.

We never call simple_server with arguments so this is dead code.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
@jku jku force-pushed the allow-compressed-metadata-in-flight branch from 84dd3de to 8340918 Compare January 21, 2022 09:31
@jku
Copy link
Member Author

jku commented Jan 21, 2022

Marking non-draft with only these changes:

@jku jku marked this pull request as ready for review January 21, 2022 09:34
socketserver.TCPServer.allow_reuse_address = True

httpd = socketserver.TCPServer(("localhost", 0), handler)
httpd = socketserver.TCPServer(("localhost", 0), SimpleHTTPRequestHandler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for cleaning up!

As the comment above says, I introduced this workaround for testing on Windows in Python2 (7dbb30a). So this has become irrelevant quite a while ago.

This commit tries to deal with two interests:
* metadata is highly repetitive and compressible: allowing compression
  would be good
* there may be broken web servers (see
  https://github.com/pypa/pip/blob/404838abcca467648180b358598c597b74d568c9/src/pip/_internal/download.py#L842)
  that have problems with compression on already compressed target files

We can make things better for that first interest while we have no real
data for the second interest -- our current workarounds to avoid
compression are based on hearsay, not testing.

Now that individual fetchers are possible I suggest we simplify
ngclient and allow compression. As an example the pip Fetcher
could still use the pip response chunking code with all their
workarounds -- pip certainly has better capability to maintain
a mountain of workarounds and also has endless amounts of real-world
testing compared to python-tuf.

Details:
* Stop modifying Accept-Encoding (Requests default includes gzip)
* Don't use response.raw in RequestsFetcher as there is no need:
  This was a workaround for false "Content-encoding: gzip" inserted by
  a broken server -- and the workaround was only possible because we
  knew we never asked for compression
* Fix issue in test_session_get_timeout(): it's not mocking the error
  that requests really raises in this case

Fixes theupdateframework#1251

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
@jku jku force-pushed the allow-compressed-metadata-in-flight branch from 8340918 to b8759a9 Compare January 25, 2022 13:07
@jku
Copy link
Member Author

jku commented Jan 25, 2022

modified commit message with an explanation of why we no longer use requests.Response.raw (which is really a urllib3.response.HTTPResponse)

@lukpueh lukpueh merged commit c6dab7e into theupdateframework:develop Jan 25, 2022
@jku jku deleted the allow-compressed-metadata-in-flight branch December 30, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Client explicitly asks server to not compress data

4 participants