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.

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

Merged
merged 5 commits into from
Nov 23, 2023
Merged

Conversation

cono
Copy link
Contributor

@cono cono commented Nov 22, 2023

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
Member

How can I reproduce this issue locally?

@cono
Copy link
Contributor Author

cono commented Nov 22, 2023

@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())

@karpetrosyan
Copy link
Member

karpetrosyan commented Nov 22, 2023

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.

@karpetrosyan karpetrosyan added the bug Something isn't working label Nov 22, 2023
@cono
Copy link
Contributor Author

cono commented Nov 22, 2023

@karpetrosyan added

Copy link
Contributor

@zanieb zanieb 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 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
Copy link
Contributor Author

cono commented Nov 22, 2023

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.

CHANGELOG.md Outdated Show resolved Hide resolved
httpcore/__init__.py Outdated Show resolved Hide resolved
@karpetrosyan
Copy link
Member

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
Copy link
Contributor Author

cono commented Nov 23, 2023

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

@cono cono closed this Nov 23, 2023
@tomchristie tomchristie reopened this Nov 23, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@tomchristie tomchristie merged commit 825e7e3 into encode:master Nov 23, 2023
5 checks passed
@tomchristie tomchristie mentioned this pull request Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants