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 connect timeouts for anyio backend #236

Merged

Conversation

cdeler
Copy link
Member

@cdeler cdeler commented Nov 16, 2020

Why to add changes

In encode/httpx#1387 (comment) I found that anyio backend has problem with connection timeouts here:

        exc_map = {
            OSError: ConnectError,
            TimeoutError: ConnectTimeout,
            BrokenResourceError: ConnectError,
        }

        with map_exceptions(exc_map):
            async with anyio.fail_after(connect_timeout):
                stream: anyio.abc.ByteStream
                stream = await anyio.connect_tcp(
                    unicode_host, port, local_host=local_address
                )
                if ssl_context:
                    stream = await TLSStream.wrap(
                        stream,
                        hostname=unicode_host,
                        ssl_context=ssl_context,
                        standard_compatible=False,
                    )

As assert isinstance(TimeoutError(), OSError), the above map_exceptions throws ConnectError in case of any timeout (at least in await TLSStream.wrap)

What has been done

I added 2 tests (test_connection_timeout_tcp and test_connection_timeout_uds) and fixed the error

Note

test_connection_timeout_uds has been marked as python >= 3.7 to reduce changeset footprint in the slow_uds_server fixture (otherwise there should be a fallback Uvicorn server)

@cdeler cdeler requested review from a team and florimondmanca November 16, 2020 09:32
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Left a comment to simplify things on the test setup side. 👍

tests/conftest.py Outdated Show resolved Hide resolved
tests/async_tests/test_interfaces.py Outdated Show resolved Hide resolved
@cdeler cdeler force-pushed the fix-connect-timeouts-for-anyio-backend branch from e1587d1 to 4fe216e Compare November 25, 2020 14:20
@cdeler
Copy link
Member Author

cdeler commented Nov 25, 2020

@florimondmanca
As at hypercorn level I cannot work at socket level (if you know how to do it, please could you share a snippet 🙏 ), e.g. I cannot "accept a connection after small delay", I had to do a work-around. I'm trying to access http server using https url. It causes a timeout inside

stream = await TLSStream.wrap(
    stream,
    hostname=unicode_host,
    ssl_context=ssl_context,
    standard_compatible=False,
)

and that is expected by me.

On other hand it means that all the work around fixtures was redundant. Should I remove all the conftest.py changes from this PR?

@florimondmanca
Copy link
Member

@cdeler Yes, if just trying to connect via https to the HTTP server is enough to trigger a timeout, then let's drop the rest and anything we don't need. I'm all for reducing the footprint of PRs whenever possible. :-)

@cdeler cdeler force-pushed the fix-connect-timeouts-for-anyio-backend branch from 9ac15aa to d003888 Compare November 26, 2020 10:52
@cdeler cdeler force-pushed the fix-connect-timeouts-for-anyio-backend branch from d003888 to 87658d1 Compare November 26, 2020 10:57
@cdeler
Copy link
Member Author

cdeler commented Nov 26, 2020

@florimondmanca I removed all unnecessary fixtures. But I still have to skip the test on 3.6 since testing against example.com behaves different

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@florimondmanca florimondmanca merged commit 8fae4aa into encode:master Nov 30, 2020
@cdeler cdeler deleted the fix-connect-timeouts-for-anyio-backend branch November 30, 2020 13:19
@florimondmanca florimondmanca mentioned this pull request Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants