Repository: sanic-org/sanic
Pull request: Increase KEEP_ALIVE_TIMEOUT default to 120 seconds
Sanic has used 5 second timeout to quickly close idle connections, for no particular reason, as idle connections use very little resources. This PR increases it to 80 seconds, slightly larger than Nginx default of 75 s, to avoid a problem with a request failing when Sanic has closed the connection but Nginx hasn't noticed. This should also improve practical performance on long latency connections, where reconnecting is expensive, and better fits typical user flow browsing pages with longer-than-5-second intervals.

The root cause of the next request failing (i.e. connection not being properly terminated) should be looked at in more detail, but meanwhile this should be a useful workaround to keep the defaults working fluently, as well as to improve performance for the aforementioned reasons.

Fix #2681

Problem statement:
Send keep-alive header and increase the default timeout
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Is your feature request related to a problem? Please describe.

Nginx doesn't notice Sanic closing its keep-alive connection by timeout, and then another request will fail for no good reason. High latency connections suffer of having to do frequent reconnections when a client or proxy loads additional resources 5+ seconds later, despite keeping such connections open being practically free with the async server.

### Describe the solution you'd like

Sanic http1 server should send `keep-alive: timeout=<KEEP_ALIVE_TIMEOUT>`, to inform the client of the duration it allows. Also Sanic should not limit max number of requests per pipeline (no reason to) and use a higher default `KEEP_ALIVE_TIMEOUT` setting of at least 70 seconds to exceed that of Nginx to avoid connection problems.

Sanic should also investigate ways of closing the connection so that Nginx or another client gets the notice of connection RST and won't attempt to send another request once those are no longer accepted. Could be problematic with TLS but this is also broken in plain HTTP in current version.

### Additional context

HTTP/3 does not use keep-alive header. On ASGI this might be up to the ASGI server, needs investigation.

Hints from issue discussion:
Related issue #2531
Yes, that is for ASGI server to handle. I do not want to change any defaults. If someone needs to change they have the ability to. When we overhaul timeouts next qtr we can address what our defaults will be.
There is no reason to keep the default so low (5 seconds currently) and there are clear performance benefits of making it longer (as it has been in past Sanic versions I believe). Set for v23.6 because v23.3 already has a lot of changes and we don't want to make it heavier, especially if there is a general timeout overhaul in v23.6 as has been planned. This is also non-breaking.
We're messing with timeouts in 23.6. I do not see a compelling reason to change it now. Yes, it is breaking. The app will change its behavior just by upgrading if you are not already making the change. I'd rather not have to mess with default behavior two releases in a row and want to minimize the number of breaking change notifications we need to provide. AFAIK this has always been the value, it has been documented, it is trivial for someone to change if they need to.

I do agree we should be setting that header. I'm surprised we are not.
@ahopkins This specifically targets v23.6 to avoid messing with it now, or twice in a row as you say. Just writing the keep-alive concerns down on this issue so that they are not forgotten by that time. The changes mentioned in this issue are not breaking (to my knowledge) but the general timeout overhaul will be.

Writing the header could be avoided (increasing performance a bit) if we had working connection RST that signaled the clients essentially the same thing. Otherwise, adding the header could get that done (I have not yet tested if it avoids the failed requests).
Didn't we include that in the header? I think this should be implemented first, at least.
Caddy (now becoming a popular replacement for Nginx) defaults to two minutes. Not sure if the header is needed, as long as the default time is higher. Might as well go for 120 seconds IMO, it doesn't really cost anything. The header costs some extra bytes and formatting time in req/s!

Commit message: 120 seconds instead, to match Caddy as well

Patch to review:
diff --git a/sanic/config.py b/sanic/config.py
index 4316773d5e..7ac1613a5e 100644
--- a/sanic/config.py
+++ b/sanic/config.py
@@ -50,7 +50,7 @@
     "INSPECTOR_TLS_KEY": _default,
     "INSPECTOR_TLS_CERT": _default,
     "INSPECTOR_API_KEY": "",
-    "KEEP_ALIVE_TIMEOUT": 5,  # 5 seconds
+    "KEEP_ALIVE_TIMEOUT": 120,  # Two minutes matches Caddy, exceeds Nginx default
     "KEEP_ALIVE": True,
     "LOCAL_CERT_CREATOR": LocalCertCreator.AUTO,
     "LOCAL_TLS_KEY": _default,


Code review comments:
diff_hunk: @@ -50,7 +50,7 @@
     "INSPECTOR_TLS_KEY": _default,
     "INSPECTOR_TLS_CERT": _default,
     "INSPECTOR_API_KEY": "",
-    "KEEP_ALIVE_TIMEOUT": 5,  # 5 seconds
+    "KEEP_ALIVE_TIMEOUT": 120,  # Two minutes matches Caddy, exceeds Nginx default
path: sanic/config.py
text: @user1:
Please take out this comment. It does not add anything here.