-
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
Catch errors setting SO_REUSEPORT #1483
Conversation
316f2a8
to
9e076f4
Compare
The py36 coverage seems totally spurious. |
Would it be possible that the tests are not guaranteed to run in a deterministic way where the code path for this branching logic could be different for some test runs? That's usually what the issue is. I've seen this same exact coverage report in other PRs of gunicorn. |
@tilgovi Just so you know, I put a breakpoint just before |
Here's what I actually ran (so I don't have to wait so long for all the tests to run): while true; do tox -e py35 -- tests/test_invalid_requests.py; done |
I think it's more deliberate than that. Search |
Hah, there you go. I think it would be a good opportunity to fix the tests so they are deterministic (and also write additional tests to cover both sides of the coin) |
gunicorn/sock.py
Outdated
try: | ||
sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1) | ||
except socket.error as err: | ||
if err[0] in (errno.ENOPROTOOPT, errno.EINVAL): |
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.
Or a simpler version:
if err[0] not in (errno.ENOPROTOOPT, errno.EINVAL):
raise
9e076f4
to
15408a3
Compare
Updated. |
try: | ||
sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1) | ||
except socket.error as err: | ||
if err[0] not in (errno.ENOPROTOOPT, errno.EINVAL): |
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'm wondering if instead of raising an error, we couldn't simply try to start without this option and log a warning. Thoughts?
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.
Looking at the errors that setsockopt(2) can return, I think anything else is probably very bad.
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.
fair enough :)
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.
It's OK with this patch~
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.
Fixes #1480 on my system.
@aconrad what do you mean by more deterministic? |
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.
let's wait if we have some feedback but 👍 for the merge anyway
@benoitc Couverture.io showed us that the code path execution of each test run may be different each time (not guaranteed, non-deterministic), likely due to the As we can see from the coverage report, line 215 or line 218 of file https://github.com/benoitc/gunicorn/blob/master/gunicorn/http/message.py#L215-L218 may be executed during test runs (unpredictable): So if the culprit is indeed the Once fixed (e.g. line 215 is guaranteed to be the code path taken) then we should write additional tests to make sure that line 218 is also covered eventually. I hope this helps. |
try: | ||
sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1) | ||
except socket.error as err: | ||
if err[0] not in (errno.ENOPROTOOPT, errno.EINVAL): |
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.
Fixes #1480 on my system.
Catch errors setting SO_REUSEPORT
Fix #1480