fix!: defer socket.connect() from __init__ to connect()#570
fix!: defer socket.connect() from __init__ to connect()#570agners wants to merge 2 commits intoBluetooth-Devices:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #570 +/- ##
==========================================
- Coverage 85.31% 84.36% -0.96%
==========================================
Files 28 28
Lines 3433 3479 +46
Branches 601 604 +3
==========================================
+ Hits 2929 2935 +6
- Misses 312 351 +39
- Partials 192 193 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #570 will not alter performanceComparing Summary
|
|
Probably most problematic is that Exceptions now get raised with try:
connected_bus = await MessageBus(bus_type=BusType.SYSTEM).connect()
except OSError as e:
...in which case it ends up not to matter. But breaking change nonetheless. |
BREAKING CHANGE: Connection errors are now raised from connect() instead of __init__. Code that catches exceptions from MessageBus() instantiation must be updated to catch them from connect() instead. Previously, BaseMessageBus.__init__() called _setup_socket() which performed a blocking socket.connect() call. This violated async design principles - an async library should not perform blocking I/O in __init__. This caused issues with tools like blockbuster that detect blocking calls in async contexts (e.g., Home Assistant Supervisor). Changes: - _setup_socket() now only creates the socket and stores the connection address, but does not connect. Socket is set to non-blocking immediately. - Added _connect_socket() for synchronous blocking connection (unused by current implementations but available for sync use cases) - aio MessageBus.connect() now uses await loop.sock_connect() for proper async socket connection - glib MessageBus.connect() now initiates non-blocking socket connection and uses a GLib source to wait for completion if needed - Added _sock_connect_address to __slots__ and .pxd file - Updated tests to expect connection errors from connect() not __init__ Fixes blocking I/O detection errors like: blockbuster.BlockingError: Blocking call to socket.socket.connect 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
agners
force-pushed
the
fix-sync-io-in-messagebus-init
branch
from
e4bc481 to
b9ada81
Compare
| return GLib.SOURCE_CONTINUE | ||
|
|
||
|
|
||
| class _ConnectSource(_GLibSource): |
There was a problem hiding this comment.
Maybe keep glib the same as it was before and only fix asyncio to contain scope. We could pass a flag to defer connect for asyncio.
I don't think any of the maintainers use the glib version so we try not to touch it
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
This isn't the worst breaking change ever since it is mostly only about where the exception is raised.
I'm not sure I like the suggestion of adding a paramter to opt into the new behavior. The new behavior is the more "correct" behavoir, we it would be best if we just did the right thing without having to depend on the user knowing what is best. We could add a deprecation warning to help users find this out if the don't set the option. But then we are stuck with the option forever, or we have to have another deprecation period and another breaking change to remove it.
Maybe we could write a quick script to check libraries that depend on dbus-fast to see if anyone is actually catching the exception from the constructor. If there aren't any, maybe we don't worry about it. Or if there are just a few, we could notify them of the change.
| @@ -726,25 +732,31 @@ def _setup_socket(self) -> None: | |||
| if "port" in options: | |||
| ip_port = int(options["port"]) | |||
|
|
|||
| try: | |||
| self._sock.connect((ip_addr, ip_port)) | |||
| self._sock.setblocking(False) | |||
| except Exception as e: | |||
| last_err = e | |||
| else: | |||
| stack.pop_all() | |||
| return | |||
| # Store connect address for later; don't connect yet | |||
| self._sock_connect_address = (ip_addr, ip_port) | |||
| self._sock.setblocking(False) | |||
| stack.pop_all() | |||
| return | |||
|
|
|||
| else: | |||
| raise InvalidAddressError( | |||
| f"got unknown address transport: {transport}" | |||
| ) | |||
| raise InvalidAddressError(f"got unknown address transport: {transport}") | |||
|
|
|||
| # Should not normally happen, but just in case | |||
| raise TypeError("empty list of bus addresses given") # pragma: no cover | |||
There was a problem hiding this comment.
Removing the try/excepts here breaks the automatic fallback to "tcp" if connecting the "unix" transport fails. This entire method will need to be moved to the connect method.
| the DBus daemon failed. | ||
| - :class:`Exception` - If there was a connection error. | ||
| """ | ||
| try: |
There was a problem hiding this comment.
Docs for this method need a ..versionchanged: X.Y.Z and to have the :raises: section updated.
And we should add ..versionchanged: X.Y.Z to class MessageBus as well.
3 participants
NOTE: I just had AI a go at #569. I guess this would need (another) major bump 😰 . Not sure if the GLib implementation really needs to be that complicated 🤷 .
BREAKING CHANGE: Connection errors are now raised from connect() instead of init. Code that catches exceptions from MessageBus() instantiation must be updated to catch them from connect() instead.
Previously, BaseMessageBus.init() called _setup_socket() which performed a blocking socket.connect() call. This violated async design principles - an async library should not perform blocking I/O in init.
This caused issues with tools like blockbuster that detect blocking calls in async contexts (e.g., Home Assistant Supervisor).
Changes:
Fixes blocking I/O detection errors like:
blockbuster.BlockingError: Blocking call to socket.socket.connect
🤖 Generated with Claude Code
Fixes: #569