-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
workers/gthread: Remove locks + one event queue + general cleanup #3157
base: master
Are you sure you want to change the base?
Conversation
17e7abf
to
04243f3
Compare
12590cd
to
1bda38f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried ab -n1000 -c100 http://0.0.0.0:8080/
with my standalone script (as explained here #3147) and it does not work well, so rejecting from my side
If it is so, then that doesn't sound good @javiertejero -- I would have thought the issue to be fixed! I can't reproduce any hanging behavior with I'm testing the standalone app using the options: options = {
'bind': '%s:%s' % ('127.0.0.1', '8080'),
'workers': 1,
'worker_class': 'gthread',
'threads': 3,
'worker_connections': 4,
} But now I have tested using Linux. Are you running MacOS? |
@sylt oh yes, apologies, I was testing with MacOS when trying on linux it works just fine as you mentioned with a huge concurrency :) however in MacOS I see this with "just" 100 concurrent clients
on the server I see this:
UPDATE: with less concurrency like 10 it works fine let me know if I can help further with this |
Thanks for confirming @javiertejero ! Based on your stack trace (thank you very much!) I think I have corrected the error. If you would have the ability to test the latest patch set, I would be more than grateful! The error was my usage of DefaultSelector().register, which has different behavior depending on if one is running Linux (which will use the EpollSelector) or Mac (which uses KqueueSelector). The former accepts to have events as set as "0" (meaning no events), while the latter doesn't. The fix is to try and not be smart, but simply use DefaultSelector().unregister when we're not interested in accepting new connections any more :) This way, we won't rely on any implementation specific behavior. Perhaps even the code got a bit clearer... |
@sylt I don't think we are allowed to call DefaultSelector.unregister twice in a row (1. bottom of loop 2. immediately after) |
@pajod Ah, yes, you're right! We could get in that situation if the server is terminated right at the time when it's very busy. I've tried to address it in the latest patch. Thanks! |
Sorry, does not merge cleanly because of my #3189 - fix is to apply those changes like this: diff --git a/gunicorn/workers/gthread.py b/gunicorn/workers/gthread.py
index 49946d77..196759b8 100644
--- a/gunicorn/workers/gthread.py
+++ b/gunicorn/workers/gthread.py
@@ -1 +0,0 @@
-# -*- coding: utf-8 -
@@ -34 +33 @@ from ..http import wsgi
-class TConn(object):
+class TConn:
@@ -145 +144 @@ class ThreadWorker(base.Worker):
- except EnvironmentError as e:
+ except OSError as e:
@@ -264 +263 @@ class ThreadWorker(base.Worker):
- except EnvironmentError as e:
+ except OSError as e:
@@ -318 +317 @@ class ThreadWorker(base.Worker):
- except EnvironmentError:
+ except OSError:
@@ -329 +328 @@ class ThreadWorker(base.Worker):
- except EnvironmentError:
+ except OSError: |
The main purpose is to remove complexity from gthread by: * Removing the lock for handling self._keep and self.poller. This is possible since we now do all such manipulation on the main thread instead. When a connection is done, it posts a callback through the PollableMethodCaller which gets executed on the main thread. * Having a single event queue (self.poller), as opposed to also managing a set of futures. This fixes benoitc#3146 (although there are more minimal ways of doing it). There are other more minor things as well: * Renaming some variables, e.g. self._keep to self.keepalived_conns. * Remove self-explanatory comments (what the code does, not why). * Just decide that socket is blocking. * Use time.monotonic() for timeouts in gthread. Some complexity has been added to the shutdown sequence, but hopefully for good reason: it's to make sure that all already accepted connections are served within the grace period.
sorry for the delay, just tested in macosx again and it works now, thanks @sylt |
The main purpose is to remove complexity from gthread by:
Removing the lock for handling self._keep and self.poller. This is possible since we now do all such manipulation on the main thread instead. When a connection is done, it posts a callback through the PollableMethodCaller which gets executed on the main thread.
Having a single event queue (self.poller), as opposed to also managing a set of futures. This fixes Gunicorn gthread deadlock #3146 (although there are more minimal ways of doing it).
There are other more minor things as well:
Some complexity has been added to the shutdown sequence, but hopefully for good reason: it's to make sure that all already accepted connections are served within the grace period.