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

GZIP content length mismatch fix #1579

Closed
wants to merge 4 commits into from
Closed

GZIP content length mismatch fix #1579

wants to merge 4 commits into from

Conversation

hiranp
Copy link

@hiranp hiranp commented Apr 7, 2022

This addresses concerns raised by the following discussions:

We encountered this problem when developing our internal application, when we implemented both Brotli and GZIP compression. We tested the following fix using several use cases.

The solution tries to address the problem using two(2) approaches:

@Kludex
Copy link
Member

Kludex commented Apr 7, 2022

Is this still an issue on fastapi/fastapi#4488 ?

@hiranp

This comment was marked as spam.

@hiranp
Copy link
Author

hiranp commented Apr 8, 2022

Also, I going to update my pull request, found another corner case. I am not handling.

@Kludex
Copy link
Member

Kludex commented Apr 10, 2022

How can I reproduce the issue here?

@hiranp
Copy link
Author

hiranp commented Apr 12, 2022

This issues seems to occur on a serving static files and this case from the linked issues from FastApi [4050]. However, I need to find time to translate my specific case into a smaller test case.


from typing import Awaitable, Callable

from fastapi import FastAPI
from fastapi.middleware.gzip import GZipMiddleware
from starlette.requests import Request

app = FastAPI()


@app.middleware("http")
async def noop_middleware(request: Request, call_next: Callable[[Request], Awaitable]):
    return await call_next(request)


@app.get("/", status_code=304)
def youve_seen_this_before():
    pass


app.add_middleware(GZipMiddleware)

@Kludex
Copy link
Member

Kludex commented Apr 12, 2022

I'm able to reproduce this with:

from typing import Awaitable, Callable

from starlette.middleware.base import BaseHTTPMiddleware
from starlette.middleware.gzip import GZipMiddleware
from starlette.routing import Route
from starlette.middleware import Middleware
from starlette.applications import Starlette
from starlette.responses import Response
from starlette.requests import Request

class NoopMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request: Request, call_next: Callable[[Request], Awaitable]):
        return await call_next(request)

def endpoint(request: Request):
    return Response(status_code=304)


app2 = Starlette(
    routes=[Route("/", endpoint=endpoint)],
    middleware=[Middleware(GZipMiddleware), Middleware(NoopMiddleware)]
)

This is more as a self note. I'll continue to check this later on.

@Kludex Kludex requested a review from abersheeran April 24, 2022 05:23
@abersheeran
Copy link
Member

This PR did solve the problem, but that # noqa made me confused. 🤔

@levrik
Copy link

levrik commented Apr 25, 2022

These seem to be linting rules coming from https://wemake-python-stylegui.de/en/latest/
Probably @hiranp has this set up globally on his machine?

starlette/middleware/gzip.py Outdated Show resolved Hide resolved
starlette/middleware/gzip.py Outdated Show resolved Hide resolved
starlette/middleware/gzip.py Outdated Show resolved Hide resolved
starlette/middleware/gzip.py Outdated Show resolved Hide resolved
@Kludex Kludex self-requested a review April 25, 2022 14:01
@Kludex Kludex added this to the Version 0.21.0 milestone Apr 25, 2022
@Kludex
Copy link
Member

Kludex commented Apr 29, 2022

This PR doesn't solve the issue for me...

I've tried with both examples above, and I still have the same issues.

@Kludex
Copy link
Member

Kludex commented Apr 30, 2022

Thanks for the PR @hiranp !

After debugging this issue, it looks like it's more related to the BaseHTTPMiddleware than the GZipMiddleware.

I'll be closing this issue, and I'd really appreciate if you can take a look at #1609. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold Don't merge it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants