[go: nahoru, domu]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't block unix sockets #54

Merged
merged 2 commits into from
Mar 24, 2021
Merged

Don't block unix sockets #54

merged 2 commits into from
Mar 24, 2021

Conversation

joetsoi
Copy link
Contributor
@joetsoi joetsoi commented Feb 25, 2021

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 an AF_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

import asynctest

class TestClass(asynctest.TestCase):
    async def test_something(self):
        pass

@joetsoi
Allow AF_UNIX sockets in GuardedSocket
5067c81 
asyncio uses unix domain sockets, as we're mainly interested in blocking
external requests, allowing AF_UNIX through should be fine.
@alex-verve
Copy link
alex-verve commented Feb 25, 2021 •
edited
Loading

Tested on five Django projects and it works like a charm 👍
I don't need to patch django via env var anymore, thanks for your work!

@miketheman miketheman added the enhancement New feature or request label Feb 26, 2021
@miketheman
Copy link
Owner

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.

@joetsoi
Copy link
Contributor Author
joetsoi commented Feb 26, 2021

happy to do both! just wanted to throw this out there before investing too much time in it

codeclimate[bot]
codeclimate bot reviewed Mar 5, 2021
View reviewed changes
pytest_socket.py Show resolved Hide resolved
@joetsoi
Copy link
Contributor Author
joetsoi commented Mar 6, 2021

Windows failure looks like an unrelated failure on setup. Not the absence of AF_UNIX that I was hoping to test

@joetsoi
Copy link
Contributor Author
joetsoi commented Mar 18, 2021

@miketheman, no pressure, just a ping hoping you find some time to take a look at this.

miketheman
miketheman reviewed Mar 20, 2021
View reviewed changes
Copy link
Owner
@miketheman miketheman left a 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!

tests/test_socket.py Outdated Show resolved Hide resolved
@miketheman miketheman self-assigned this Mar 20, 2021
@miketheman
Copy link
Owner

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!

@joetsoi joetsoi force-pushed the master branch 2 times, most recently from 01e04fc to ee3fabf Compare March 23, 2021 14:33
@codeclimate
Copy link
codeclimate bot commented Mar 23, 2021

Code Climate has analyzed commit 197d3e3 and detected 0 issues on this pull request.

View more on Code Climate.

@miketheman miketheman merged commit 305b77e into miketheman:master Mar 24, 2021
@miketheman
Copy link
Owner

Thanks for sticking with it!
I’ll try to get to a release next week.

This was referenced Mar 28, 2021
Closed
Closed
miketheman added a commit that referenced this pull request Mar 30, 2021
@miketheman
docs: update for unix sockets
d819f95 
In #54 this flag was added, and let's document it.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
miketheman added a commit that referenced this pull request Mar 30, 2021
@miketheman
test: add testing for some async frameworks
74f3177 
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 <miketheman@gmail.com>
@miketheman miketheman mentioned this pull request Mar 30, 2021
Merged
miketheman added a commit that referenced this pull request Mar 30, 2021
@miketheman
test: add testing for some async frameworks
ea63f63 
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 <miketheman@gmail.com>
miketheman added a commit that referenced this pull request Mar 30, 2021
@miketheman
test: add testing for some async frameworks
15a3714 
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 <miketheman@gmail.com>
@sbrunato sbrunato mentioned this pull request Apr 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate left review comments

@miketheman miketheman miketheman approved these changes

Assignees

@miketheman miketheman

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@joetsoi @alex-verve @miketheman