-
Notifications
You must be signed in to change notification settings - Fork 100
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
Fix bug with async socks proxy and tracing. #849
Conversation
How can I reproduce this issue locally? |
@karpetrosyan use async interface and socks5 proxy. e.g.: #! /usr/bin/env python
import asyncio
import httpx
async def trace_func(event, data):
print(f'{event=} {data=}')
async def make_request():
async_client = httpx.AsyncClient(
proxies={'all://': 'socks5://non-existed-domain.com'},
)
async with async_client as client:
response = await client.request(
"GET", "https://www.encode.io", extensions={"trace": trace_func}
)
print(response)
asyncio.run(make_request()) |
Yes, I see the problem. We also need a changelog for this PR. |
@karpetrosyan added |
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 contributing!
Can you add test coverage for this?
I'm also not sure we should bump the version here, that's usually done in a separate pull request. You can just use an unreleased section for the entry.
I wonder why person who added this code haven't covered this with tests. |
The code is actually covered, at least, because we have 100% code coverage.
If you have found a bug that has not been tested and you have considered opening the pull request to fix that bug, you MUST add a test case, especially when it was requested by the maintainer. There are rules for how this project should be maintained. Anyway, I think we don't need a test case for this pull request. Maybe we could add a rule to unasync.py so we can avoid this kind of situation. If @zanieb is okay with that, we can skip the test case for this change. Thanks for this pull request @cono 🙏 |
Guys, you have a bug in a library which crash application in particular circumstances, and isntead of accepting 2 line changes, you making discussion for 2 days about nothing. It's kinda like a joke to me. |
Summary
If you are using socks proxy, asynchronous client and tracing, it fails with the following error:
Checklist