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

Prevent gthread connection reset on max-requests restart #3039

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

christianbundy
Copy link

@christianbundy christianbundy commented Jul 21, 2023

Background

We want to support TCP connections that haven't sent data, so 0ebb73a was merged to remove the assumption that all connections are readable. Instead of reading from the connection immediately, we use the selector event loop to determine when to read from the connection. This means that the while self.alive loop in self.run() needs to run twice to handle a single HTTP request:

  1. The first time through the loop we call self.poller.select() and see the 'listener is readable' event, which prompts us to call self.accept(). This creates a connection, but does not read from it. Instead,self.accept() calls self.poller.register() so that the connection is handled as soon as it's readable.
  2. The second time through the loop we run self.poller.select() again, but this time we'll see the 'connection is readable' event, which prompts us to call self.on_client_socket_readable(). This handles the HTTP request and sends a response.

Problem

We reset connections when we call accept() on connections, but not recv(). This happens when we reach self.max_requests and set self.alive = False, which prevents us from continuing through the while self.alive loop.

There are two related bugs, which are also important to fix:

  • The maximum connection limit is not respected. We ensure that self.nr_conns < self.worker_connections before running any callbacks, but consider the case where self.nr_conns == 9, self.worker_connections == 10, and we receive twenty callbacks to accept connections.
  • Speculative TCP connections can cause a deadlock. We only call self.poller.select() if we pass the self.nr_conns < self.worker_connections condition, so if we hit the limit with only speculative TCP requests then we'll never read any data from the connections.

Solution

Revert 0ebb73a. I've tried a handful of different clever ways of resolving this, but I haven't been able to find one, and 'the server doesn't reset connections on restart' is significantly more important to me than 'the server can hold lots of speculative TCP connections'.


Fixes: #3038

@christianbundy christianbundy marked this pull request as draft September 8, 2023 15:19
@benoitc benoitc self-assigned this Dec 7, 2023
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

Successfully merging this pull request may close these issues.

Connection reset during max-requests auto-restart with gthread
2 participants