Skip to content
This repository has been archived by the owner on Nov 23, 2017. It is now read-only.

Pause server #448

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Pause server #448

wants to merge 5 commits into from

Conversation

mayfield
Copy link

Re: https://groups.google.com/forum/?utm_source=digest&utm_medium=email#!topic/python-tulip/btGHbh5kUUM

Tests pass. I sort of wish the pause and resume functions could elegantly live on the event loop and not on the Server object, but it seems to be the best contextual storage for all the listening sockets.

Also I'm not sure max_connections on create_server is justifiable or even a good name, but it was immediately useful to my particular problem and I imagine it could be to others too. So I left it in.

Add a `pause`/`resume` API to `Server` which removes/adds a server's
listening sockets on its event loop selector.  This facility allows DoS
prevention from SYN flooding connection herds.

Add `max_connections` kwarg to `loop.create_server` and `Server` which
controls pause/resume behavior when not `None`.

Notes:

  1. Using Server.pause/resume and create_server(max_connections) are
     mutually exclusive.
  2. The listen backlog and accept semantics are not taken into consideration.
     As a result the actual number of connections established will vary
     and should be considered platform and/or event loop dependant.
Fix Server create for unix sockets and add support for max_connections.
@1st1
Copy link
Member

1st1 commented Oct 19, 2016

We'll need tests for this new functionality. I think that max_connections functionality is neat, but what's the use case again?

@mayfield
Copy link
Author

@1st1 max_connections just says stop accepting new connections when the current active connection count is number. I have a proxy service that handles statsd and diamond data streams from thousands of clients, then adds some metadata and forwards to a wavefront agent. The ingress rate often exceeds the egress potential because of outside factors. Without a connection limit the server happily accepts new connections even while its egress rate plummets due to resource contention. My initial attempt to control backpressure with a semaphore on the recv side of the proxy wasn't good enough because I can't tell the event loop to stop accepting new connections. It only takes a few seconds to eat gigabytes of memory under the right conditions.

@1st1
Copy link
Member

1st1 commented Oct 19, 2016

But what criteria do you use to pick up a sound number for max_connections? Is it just some arbitrary number, or there is a way to calculate an optimal one for the given system?

@mayfield
Copy link
Author

I'd expect it to be mostly application dependant. It would obviously need to be less than the nofile ulimit for the user running the process and then under the kernel's max open file limit and finally it needs to be a number that prevents memory exhaustion. The latter of which is going to vary depending on the memory footprint of each connection in a user's application.

In my particular case setting max_connections to 5000 puts my memory ceiling at ~ 100MB. But I actually get better performance with a lower number. Setting it too low however, can cause degraded service to clients. There are tradeoffs everywhere, hence a default of None (no limit).

@mayfield
Copy link
Author

I'm writing some tests btw, but before I get too far, are you generally okay with the location of the functionality and the way the start_serving arguments are stored in Server? I frankly couldn't convince myself that I was using idiomatic conventions with that stuff. It's not clear to me when a user should interact with a Server object vs using event loop functions since most behavior is implemented in the event loop and the Server object seems to be just a placeholder for sockets and cleanup.

@1st1
Copy link
Member

1st1 commented Oct 19, 2016

I'm writing some tests btw, but before I get too far, are you generally okay with the location of the functionality and the way the start_serving arguments are stored in Server?

I think that the way the patch is stuctured now is fine. But before you get too far, let's wait until @gvanrossum gives this idea a green light.

Tests for create_server(max_connections=number) and Server.pause/resume.
Refactored a bit to share most the logic with the TCP server.
@mayfield
Copy link
Author

mayfield commented Nov 3, 2016

Should I be evangelizing this? I'd like to see it get accepted. @gvanrossum has publicly stated he's not focused on asyncio as much as he used to be, so perhaps there are other stakeholders we can bother?

@1st1
Copy link
Member

1st1 commented Nov 4, 2016

@gvanrossum What do you think on this one? 3.7?

@gvanrossum
Copy link
Member

Definitely 3.7. Until then this could easily be implemented as a separate AbstractServer subclass.

@mayfield
Copy link
Author

mayfield commented Nov 4, 2016

3.7 is fine, let me know how I can help or what I should do, if anything.

Re: subclassing AbstractServer, I believe I brought this up in the forum in the context of allowing a user to bring their own Server subclass to start_server but it was shot down. As it's architected now, I don't see how you can bring your own Server subclass without copypasta. But I may have missed something important.

@1st1
Copy link
Member

1st1 commented Nov 4, 2016

Re: subclassing AbstractServer, I believe I brought this up in the forum in the context of allowing a user to bring their own Server subclass to start_server but it was shot down. As it's architected now, I don't see how you can bring your own Server subclass without copypasta. But I may have missed something important.

@gvanrossum This is something we can still do in 3.6, right? IIUC we can add a server_factory parameter to loop.create_server?

@gvanrossum
Copy link
Member

gvanrossum commented Nov 4, 2016 via email

@1st1
Copy link
Member

1st1 commented Mar 2, 2017

Please reopen this PR at https://github.com/python/cpython. This repo is going to be deleted soon.

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

Successfully merging this pull request may close these issues.

None yet

5 participants