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

Timeout without keep-alive #1198

Closed
imbolc opened this issue Sep 21, 2016 · 9 comments
Closed

Timeout without keep-alive #1198

imbolc opened this issue Sep 21, 2016 · 9 comments
Labels

Comments

@imbolc
Copy link
Contributor

imbolc commented Sep 21, 2016

Should not here be a check if keep-alive mode is active? https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/server.py#L227

@fafhrd91
Copy link
Member

this is different, it is slow request check. client can abuse connection and send header very slowly (kind of ddos)

@imbolc
Copy link
Contributor Author

imbolc commented Sep 21, 2016

I mean if keep-ailve mode is off, shouldn't we count here only self._slow_request_timeout?

@fafhrd91
Copy link
Member

i think it is just complicates code without benefits, also we run this code on first request as well when keep-alive state is not defined yet

@imbolc
Copy link
Contributor Author

imbolc commented Sep 21, 2016

I mean the case in which tcp_keepalive disabled in app.make_handler. E.g.

app.make_handler(
    tcp_keepalive=Flase,
    slow_request_timeout=30
)

If I do not mention keepalive_timeout in this config it will be 75. So max(self._slow_request_timeout, self._keepalive_timeout) will be 75. But I expect 30.

@imbolc
Copy link
Contributor Author

imbolc commented Sep 21, 2016

Maybe tcp_keepalive is redundant, why don't use keepalive_timeout=0 instead?

@fafhrd91
Copy link
Member

actually tcp_keepalive is only viable option for aiohttp. keepalive_timeout is incredibly slow under load.

@asvetlov
Copy link
Member

asvetlov commented Sep 21, 2016

Handling keep-alives is required for supporting HTTP persistent connections.
But tcp_keepalive is very different, it works on TCP socket keepalive level (http://www.tldp.org/HOWTO/html_single/TCP-Keepalive-HOWTO/)

Thus we need both.

@imbolc
Copy link
Contributor Author

imbolc commented Sep 21, 2016

Understood, thanks :)

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants