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

The Sanic built-in server blocks for exactly 90 seconds on status code 412 #2823

Open
1 task done
ekzhang opened this issue Sep 20, 2023 · 3 comments · Fixed by #2824
Open
1 task done

The Sanic built-in server blocks for exactly 90 seconds on status code 412 #2823

ekzhang opened this issue Sep 20, 2023 · 3 comments · Fixed by #2824
Labels

Comments

@ekzhang
Copy link
Contributor

ekzhang commented Sep 20, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Sanic, like most web servers, usually responds to requests. However, if the response has status code 412, it is very slow and takes exactly 90 extra seconds to respond, stalling after the handler finishes.

This behavior does not happen when running Sanic with uvicorn. Only the official Sanic server. It also doesn't happen with FastAPI.

Code snippet

import sanic

app = sanic.Sanic(__name__)

@app.get("/")
async def root(req: sanic.Request):
    status = int(req.args.get("status", "200"))
    return sanic.json({"message": "Hello World"}, status=status)

Expected Behavior

sanic main:app --port 8051

Then:

$ curl http://localhost:8051
{"message":"Hello World"}
$ curl http://localhost:8051/?status=400  # fine
{"message":"Hello World"}
$ curl http://localhost:8051/?status=411  # fine
{"message":"Hello World"}
$ curl http://localhost:8051/?status=413  # fine
{"message":"Hello World"}
$ curl http://localhost:8051/?status=412
# stalls with no response for 90 seconds

How do you run Sanic?

Sanic CLI

Operating System

Linux

Sanic Version

Sanic v23.6.0

Additional context

I have reproduced this on both Linux and macOS. I have also reproduced this using both the Sanic CLI and the Sanic.serve() function programmatically.

@ekzhang ekzhang added the bug label Sep 20, 2023
@Tronic
Copy link
Member

Tronic commented Sep 20, 2023

Thanks for the report. This affects HTTP status 304 and 412, which are not supposed to have "entity headers" (https://datatracker.ietf.org/doc/html/rfc2616#section-7.1).

The response gets sent immediately (use curl -v) but without content-length because that header gets stripped off by Sanic but it still sends the body bytes. Curl sees the body but doesn't know what to do with it and waits for disconnection to signal the end of it.

Triaging:
sanic/helpers.py - stripping off entity headers
sanic/response/types.py - handling of 304, 412 by above
sanic/http/http1.py:343 - gets trimmed headers and then sends body anyway

Notes: transfer-encoding: chunked is not being stripped as entity header. Due to this a response that looks like chunked encoding, given that header is received by Curl decoded 🥶

return sanic.text("4\r\nBUGS\r\n0\r\n\r\n", status=412, headers={"transfer-encoding": "chunked"})

I believe the 412 handling of Sanic as it is now is broken and handled in wrong location, causing this confusion within HTTP protocol (at least HTTP1, didn't check HTTP3 or ASGI). For those status codes in addition to entity headers also transfer-encoding and the body data would need to be stripped, it seems.

Referring to @ahopkins for further review.

@Tronic
Copy link
Member

Tronic commented Sep 20, 2023

Status code 304 is NOT affected because it is correctly handled by sanic.helpers.has_message_body.

Checking the HTTP RFC, I cannot find why entity headers or body should be stripped off 412 responses. Removing that stripping would allow body in such responses, which seems correct.

@ekzhang
Copy link
Contributor Author

ekzhang commented Sep 20, 2023

Thanks for finding the issue and responding so quickly — makes sense. I wonder why specifically 412 was treated differently. :)

The response gets sent immediately (use curl -v) but without content-length because that header gets stripped off by Sanic but it still sends the body bytes. Curl sees the body but doesn't know what to do with it and waits for disconnection to signal the end of it.

Makes sense then, so the 90 seconds is a TCP timeout.

Tronic added a commit that referenced this issue Sep 20, 2023
ahopkins added a commit that referenced this issue Jan 9, 2024
* Add test for issue #2823

* Do not strip entity-headers.

* Remove unused import

* Remove remove_entity_headers()

* Add workflow_dispatch release

* RTD build fix

* RTD build fix (another)

* RTD build fix (yet another)

* Update Crowdin configuration file

* Fix Docker publish (#2887)

* Fix Docker publish

* Remove workflow dispatch

The actions uses data from the release object itself, so workflow dispatch doesn't work anyway

* More fixes

* Remove test for removed functionality.

---------

Co-authored-by: L. Kärkkäinen <[email protected]>
Co-authored-by: Adam Hopkins <[email protected]>
Co-authored-by: Néstor Pérez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants