Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add server capability to check for Brotli compressed static files #8063

Merged
merged 21 commits into from
Feb 14, 2024

Conversation

steverep
Copy link
Member

@steverep steverep commented Jan 26, 2024

What do these changes do?

Currently server only checks if static routes have a .gz extension and serves them with gzip encoding. These changes do the same for .br files with br encoding. Brotli is prioritized over gzip if both exist and are supported by the client, as it should almost always be a smaller content length.

I considered making a check for which is smaller if both exist, but figured it wouldn't be worth the extra file system call in the vast majority of cases (at least not for typical web formats). Users should simply use gzip if it's smaller than Brotli for any file.

Are there changes in behavior for the user?

Only change would be if users are already including .br files, then obviously those would start being served, but IMO that's not "breaking" since the content should be the same.

Related issue number

Fixes #8062

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 26, 2024
aiohttp/web_response.py Outdated Show resolved Hide resolved
aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Member

bdraco commented Jan 26, 2024

I think this one should be safe for backport.

bdraco
bdraco previously requested changes Jan 26, 2024
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Please see comments above

aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
aiohttp/web_response.py Outdated Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

@bdraco I'd suggest this should only go to 3.10 backport. We try to stick to bug fixes in the patch releases.

aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
@steverep

This comment was marked as resolved.

steverep and others added 4 commits January 30, 2024 04:56
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <sviat@redhat.com>
tests/hello.txt.br Outdated Show resolved Hide resolved
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Blockers so far:

  • new binary blob in Git
  • formatting changes in tests

Please, deal with this to making this mergeable.

@bdraco bdraco dismissed their stale review February 2, 2024 19:34

Looks like all of my comments have been resolved so that leaves #8063 (review)

aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
aiohttp/web_fileresponse.py Outdated Show resolved Hide resolved
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM, #8136 should merge first so it can be backported

@steverep steverep requested a review from webknjaz February 14, 2024 03:57
@webknjaz webknjaz merged commit dfc9296 into aio-libs:master Feb 14, 2024
31 of 34 checks passed
Copy link
Contributor

patchback bot commented Feb 14, 2024

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply dfc9296 on top of patchback/backports/3.10/dfc92967d10eb83a8d726c02c3de90da15f8335f/pr-8063

Backporting merged PR #8063 into master

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/aio-libs/aiohttp.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/3.10/dfc92967d10eb83a8d726c02c3de90da15f8335f/pr-8063 upstream/3.10
  4. Now, cherry-pick PR Add server capability to check for Brotli compressed static files #8063 contents into that branch:
    $ git cherry-pick -x dfc92967d10eb83a8d726c02c3de90da15f8335f
    If it'll yell at you with something like fatal: Commit dfc92967d10eb83a8d726c02c3de90da15f8335f is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x dfc92967d10eb83a8d726c02c3de90da15f8335f
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Add server capability to check for Brotli compressed static files #8063 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.10/dfc92967d10eb83a8d726c02c3de90da15f8335f/pr-8063
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@webknjaz
Copy link
Member

@steverep looks like cherry-pick still has conflicts that need to be addressed manually. Please submit a backport PR per instructions above.

@steverep steverep deleted the serve-brotli-files branch February 14, 2024 13:08
steverep added a commit to steverep/aiohttp that referenced this pull request Feb 14, 2024
…les (aio-libs#8063)

Currently server only checks if static routes have a `.gz` extension and
serves them with `gzip` encoding. These changes do the same for `.br`
files with `br` encoding. Brotli is prioritized over gzip if both exist
and are supported by the client, as it should almost always be a smaller
content length.

I considered making a check for which is smaller if both exist, but
figured it wouldn't be worth the extra file system call in the vast
majority of cases (at least not for typical web formats). Users should
simply use gzip if it's smaller than Brotli for any file.

Resolves aio-libs#8062

(cherry picked from commit dfc9296)

Co-authored-by: Steve Repsher <steverep@users.noreply.github.com>
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: Sviatoslav Sydorenko <sviat@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serve static Brotli compressed files
4 participants