-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Don't block unix sockets #54
Conversation
asyncio uses unix domain sockets, as we're mainly interested in blocking external requests, allowing AF_UNIX through should be fine.
Tested on five Django projects and it works like a charm 👍 |
Excellent, thanks for taking this on! I think this was recently enabled by converting this to a class, so yay! I'm currently moving, so haven't had a change to check it out more thoroughly, but could you consider adding your test case to the tests so the functionality is exercised by CI? Also, had you considered making this a configuration flag like --allow-unix-sockets so that the behavior can be controlled by the user? This way it's opt-in, and not a change of the current default behavior. |
happy to do both! just wanted to throw this out there before investing too much time in it |
Windows failure looks like an unrelated failure on setup. Not the absence of AF_UNIX that I was hoping to test |
@miketheman, no pressure, just a ping hoping you find some time to take a look at this. |
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.
Thanks for adding tests and a toggle! Change requested in the test logic - see inline!
I also reran the test suite and the failing Windows test passed, so that's something we should keep an eye on - nothing worse than a flaky test! |
01e04fc
to
ee3fabf
Compare
Code Climate has analyzed commit 197d3e3 and detected 0 issues on this pull request. View more on Code Climate. |
Thanks for sticking with it! |
In #54 this flag was added, and let's document it. Signed-off-by: Mike Fiedler <[email protected]>
While the basics of asyncio ought to be covered by enabling unix sockets in #54, I thought it might be nice to add some explicit asyncio tests to ensure that we don't hit any framework-specific regressions. I had written these locally when testing the changes anyhow. Refs: #6 Refs: #47 Signed-off-by: Mike Fiedler <[email protected]>
While the basics of asyncio ought to be covered by enabling unix sockets in #54, I thought it might be nice to add some explicit asyncio tests to ensure that we don't hit any framework-specific regressions. I had written these locally when testing the changes anyhow. Refs: #6 Refs: #47 Signed-off-by: Mike Fiedler <[email protected]>
While the basics of asyncio ought to be covered by enabling unix sockets in #54, I thought it might be nice to add some explicit asyncio tests to ensure that we don't hit any framework-specific regressions. I had written these locally when testing the changes anyhow. Refs: #6 Refs: #47 Signed-off-by: Mike Fiedler <[email protected]>
This is a pretty dumb implementation and I'm happy to rework it to be a bit safer/defensive, just wanted to test if it worked first. It fixes issues with
asyncio
by check if it's anAF_UNIX
domain socket being requested and allowing that, otherwise it blocks as normal.should help with
#47
#6
I've tested it with both the starlette example and the
asynctest
example and both work with this patch