-
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
Enable SO_REUSEPORT socket option. #1131
Conversation
try: | ||
sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1) | ||
except socket.error as ex: | ||
if ex.errno == 92: |
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'd use errno.ENOPROTOOPT
instead of 92
.
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.
Also, perhaps we should raise the exception if exc.errno
is not 92
.
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.
@berkerpeksag in which case it isn't?
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.
Even if it returns other than ENOPROTOOPT (e.g. EBADF, EINVAL), we could catch them in line 40 so ignore my last comment :)
@smilliken interested by making these changes? |
There's been some silence here from @smilliken. If we're happy with this, we should just make the small changes and merge a copy of it, with attribution. |
+1 |
+1 |
which changes are needed? If someone can update the PR so we can merge it it would be helpful :) |
I have some doubt about adding reuseport support for gunicorn.
The only use case I can think of is (even more) graceful restart(starting another gunicorn process tree, using SO_REUSEPORT, then kill the old one), but gunicorn already has support for reloading worker processes... But of course, adding |
Superseded by #1344. |
Needs: