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

Connection limiting doesn't do what it says. #242

Open
Davidde94 opened this issue Sep 15, 2020 · 3 comments
Open

Connection limiting doesn't do what it says. #242

Davidde94 opened this issue Sep 15, 2020 · 3 comments

Comments

@Davidde94
Copy link

If I create a HTTPServer with a connection limit of 2, then I would expect only 2 connections to be made. However, the current implementation allows essentially an unlimited number of connections, subject only to OS limits.

This appears to be because the connection limit logic occurs inside HTTPRequestHandler, an InboundChannelHandler. By definition, the connection must exist for the execution to reach this point. Correct me if I'm wrong, but what I think you're trying to achieve is to limit the number of concurrent requests, not literal connections?

The limiting logic also appears to be evaluated at the end of a HTTP request. This is a fairly major issue, as it allows me to dump massive requests before you try and stop me. At the very least it would be a good idea to move the logic to the very start of "channelRead" to shut the connection down before data is sent.

Hope that helps, me and the rest of the NIO are more than happy to help at any time :)

@mbarnach
Copy link
Member

Thanks for your comments @Davidde94 !
Is this PR what you mentioned by moving it to the start of the logic?
An additional question: is channelInactive called when I close the connection?
Eg. do I need to decrease manually the counter everywhere context.close() is called or is it automatic?

Thanks.

@Davidde94
Copy link
Author

@mbarnach that's right! channelInactive will be called automatically

@mbarnach
Copy link
Member

Thanks! Then, I think the PR should be OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants