Skip to content

PYTHON 5212 - Use asyncio.loop.sock_connect in _async_create_connection #2383

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

Merged
merged 10 commits into from
Jun 13, 2025
Merged

PYTHON 5212 - Use asyncio.loop.sock_connect in _async_create_connection #2383

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d1cc4f9
PYTHON 5212 - Use asyncio.loop.sock_connect in _async_create_connection
NoahStapp Jun 12, 2025
5a87b91
Update unix sockets
NoahStapp Jun 12, 2025
3521829
Asynchronous timeout
NoahStapp Jun 12, 2025
9bb2e6d
Merge branch 'master' into PYTHON-5212-1
NoahStapp Jun 12, 2025
9ce66b9
Fix asyncio.TimeoutError handling
NoahStapp Jun 12, 2025
4e21b63
Restore socket timeout after connection + add changelog
NoahStapp Jun 13, 2025
af944e7
Add regression test
NoahStapp Jun 13, 2025
92fae2c
Fix typing
NoahStapp Jun 13, 2025
e08da84
Cleanup
NoahStapp Jun 13, 2025
73d41e1
Address review
NoahStapp Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pymongo/pool_shared.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,8 @@ async def _async_create_connection(address: _Address, options: PoolOptions) -> s
sock.settimeout(timeout)
sock.setsockopt(socket.SOL_SOCKET, socket.SO_KEEPALIVE, True)
_set_keepalive_times(sock)
sock.connect(sa)
sock.setblocking(False)
await asyncio.get_running_loop().sock_connect(sock, sa)
Copy link
Member

Choose a reason for hiding this comment

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

We need to do the same above for unix sockets as well.

Copy link
Member

@ShaneHarvey ShaneHarvey Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also don't we need to add more code to emulate the connect_timeout behavior now that we're using sock.setblocking(False)? Or is the async connect timeout behavior handled at a higher level?

Copy link
Contributor Author

@NoahStapp NoahStapp Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes: we'll have to move all socket timeout behavior out of the socket itself and into the AsyncNetworkInterface. In the middle of that now. We already do this.

Copy link
Contributor Author

@NoahStapp NoahStapp Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, specifically for encryption _async_configured_socket raw sockets. We manage AsyncNetworkInterface timeouts at a higher level already. Incorrect, we already handle raw async socket timeouts correctly to account for this change. The socket only needs to be nonblocking for creation.

Copy link
Member

@ShaneHarvey ShaneHarvey Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that _async_create_connection is called before AsyncNetworkInterface is ever created. So the timeout needs to be enforced here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I misunderstood what you meant. My next commit already addresses this with an asyncio.wait_for.

return sock
except OSError as e:
err = e
Expand Down
Loading