python / asyncio

asyncio historical repository
https://docs.python.org/3/library/asyncio.html
1.04k stars 177 forks source link

Check to see if SO_REUSEPORT is usable and not just defined. #418

Closed sethmlarson closed 8 years ago

sethmlarson commented 8 years ago

Some platforms define SO_REUSEPORT but do not implement it, resulting in cryptic OSErrors when trying to use socket.SO_REUSEPORT. This PR would solve that issue for all platforms that implement but don't define a usable SO_REUSEPORT. More info in Python issue 26858. See issue #352 .

Edit: I have signed the PSF Contributor Agreement

gvanrossum commented 8 years ago

I think those tests need to be fixed then. There should be only one way to test for SO_REUSEPORT.

sethmlarson commented 8 years ago

@gvanrossum Alternative is to do the check inline when the reuse_port option is specified? Wouldn't be a performance hit as create_server and create_unix_server probably get used once and forgotten.

gvanrossum commented 8 years ago

Hm, maybe just catch the exception and translate it into the same ValueError (actually a slightly similar message so we can tell the difference).

sethmlarson commented 8 years ago

@gvanrossum I switched to catching the exception inline, I also added a test to test an ill-defined SO_REUSEPORT as with the issue this PR fixes.

gvanrossum commented 8 years ago

LGTM now. @1st1 what do you think?

1st1 commented 8 years ago

LGTM. Can we merge this after b1?

sethmlarson commented 8 years ago

This also closes issue #352 as mentioned above. Can be closed if accepted but awaiting merge?

gvanrossum commented 8 years ago

Will merge (here and in CPython) after 3.6b1.

1st1 commented 8 years ago

Pushed in 610c03fb9bb238a2c1df2e6f3974bb374be3e499. Thanks!

1st1 commented 8 years ago

See also https://hg.python.org/cpython/rev/c1c247cf3488/. Looks like this is your first commit to CPython, congrats if so!

BTW, did you sign Python Contributor Agreement?

sethmlarson commented 8 years ago

@1st1 Thank you! This is my first commit and I signed it the day I submitted this PR. See my comment above in the PR.

Dakkaron commented 7 years ago

I am sorry, but to me this fix actually causes more problems than it fixes.

Yes, the old error message is rather cryptic and the new exception message is way better, but changing the exception type seems wrong to me. Yes, ValueError might fit better, but changing the type will cause a lot of problems.

My recommendation would be to catch the old OSError and then raise a new OSError with the improved error message.

sethmlarson commented 7 years ago

@Dakkaron I made this change for consistency reasons so that trying to use reuse_port on a unsupporting platform and a platform that advertises support but is actually horribly broken raise the same error type. I didn't see the value in having to catch two different error types for a very similar situation (ie you can't use SO_REUSEPORT).

If you think this should be changed you can raise an issue on the Python tracker.

Dakkaron commented 7 years ago

Sorry, my bad, I didn't notice that the old version already raised a ValueError, so your version just removes one exception type. So your change does actually help.

sethmlarson commented 7 years ago

Glad I could clarify. :)