Skip to content

Fix bug with async socks proxy and tracing. - #849

Merged
lovelydinosaur merged 5 commits into
encode:masterfrom
cono:master
Nov 23, 2023
Merged

Fix bug with async socks proxy and tracing.#849
lovelydinosaur merged 5 commits into
encode:masterfrom
cono:master

Conversation

@cono

@conocono commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Summary

If you are using socks proxy, asynchronous client and tracing, it fails with the following error:

 File "/opt/.../3rdparty_python_httpcore/site-packages/httpcore/_async/socks_proxy.py", line 231, in handle_async_request
with Trace("connect_tcp", logger, request, kwargs) as trace:
File "/opt/.../3rdparty_python_httpcore/site-packages/httpcore/_trace.py", line 50, in __enter__
self.trace(f"{self.name}.started", info)
File "/opt/.../3rdparty_python_httpcore/site-packages/httpcore/_trace.py", line 33, in trace
raise TypeError(
TypeError: If you are using a synchronous interface, the callback of the `trace` extension should be a normal function instead of an asynchronous function.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@karpetrosyan

Copy link
Copy Markdown
Contributor

How can I reproduce this issue locally?

@cono

cono commented Nov 22, 2023

Copy link
Copy Markdown
ContributorAuthor

@karpetrosyan use async interface and socks5 proxy.

e.g.:

#! /usr/bin/env pythonimportasyncioimporthttpxasyncdeftrace_func(event, data):
print(f'{event=}{data=}')
asyncdefmake_request():
async_client=httpx.AsyncClient(
proxies={'all://': 'socks5://non-existed-domain.com'},
)
asyncwithasync_clientasclient:
response=awaitclient.request(
"GET", "https://www.encode.io", extensions={"trace": trace_func}
)
print(response)
asyncio.run(make_request())

@karpetrosyan

karpetrosyan commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Yes, I see the problem.
We appear to have accidentally used the synchronous context manager in the asynchronous code.
Thank you 🙏

We also need a changelog for this PR.

@karpetrosyankarpetrosyan added the bug Something isn't working label Nov 22, 2023
@cono

cono commented Nov 22, 2023

Copy link
Copy Markdown
ContributorAuthor

@karpetrosyan added

@zaniebzanieb left a comment

Copy link
Copy Markdown
Contributor

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.

@cono

cono commented Nov 22, 2023

Copy link
Copy Markdown
ContributorAuthor

I wonder why person who added this code haven't covered this with tests.
So will leave it for you guys.
We workarounded this in production anyway, as your release process is not fast.

Comment threadCHANGELOG.md Outdated
Comment threadhttpcore/__init__.py Outdated
@karpetrosyan

Copy link
Copy Markdown
Contributor

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.

So will leave it for you guys.

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.
You cannot do just a part of the pull request and let the rest of the work be done by others.
If you had considered opening the pull request, you should either do it fully or don't do it at all.

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 🙏

@cono

cono commented Nov 23, 2023

Copy link
Copy Markdown
ContributorAuthor

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.
Do whatever you want. "MUST" is too strong for me, I don't owe you anything. Good luck

@conocono closed this Nov 23, 2023
Comment threadCHANGELOG.md Outdated
@lovelydinosaur
lovelydinosaur merged commit 825e7e3 into encode:masterNov 23, 2023
@lovelydinosaurlovelydinosaur mentioned this pull request Feb 12, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugSomething isn't working

Development

Successfully merging this pull request may close these issues.

4 participants

@cono@karpetrosyan@lovelydinosaur@zanieb