fix!: defer socket.connect() from __init__ to connect()

[agners]

Fixes #569.

Summary

The aio MessageBus constructor used to perform a blocking socket.connect() inside BaseMessageBus.__init__() (via _setup_socket). That violates async hygiene and trips blocking-call detectors like blockbuster in async contexts (notably Home Assistant Supervisor).

This PR moves socket creation and connection for the aio bus into MessageBus.connect(), so MessageBus() is now side-effect-free and connection errors surface from connect() instead of construction. The glib bus is intentionally left with its blocking-in-__init__ behavior — only the shared helper has been factored out — because we don't know who relies on the existing glib semantics and didn't want to break them on a hunch.

BREAKING CHANGE (aio only)

Connection errors that previously came from aio.MessageBus(...) instantiation are now raised from connect(). Code using the common pattern is unaffected:

bus = await MessageBus(bus_type=BusType.SYSTEM).connect()  # still works, exceptions just come from .connect()

Code that wraps the constructor itself in try/except needs to move the try to cover connect(). The glib bus is unchanged.

Approach

• Shared helper: BaseMessageBus._create_socket_for_transport(transport, options) (@staticmethod) creates an unconnected socket+stream and returns the address. Used by both aio.MessageBus.connect() and glib.MessageBus.__init__(). Options are parsed before any resources are allocated so malformed addresses fail without leaking.
• aio connect(): iterates transports with await loop.sock_connect(), uses ExitStack per attempt so a CancelledError doesn't leak the freshly-created sock/stream. The unix→tcp fallback is preserved; on total failure the last error is raised (a REVISIT: comment notes we could surface all of them via ExceptionGroup once Python 3.10 is dropped).
• aio connect() post-connect cleanup: writer creation, _authenticate(), and reader registration are wrapped in try/except BaseException that closes the socket/stream and clears _sock/_stream/_fd/_writer on failure, so an auth error no longer leaks the socket.
• aio _writer: constructed inside connect() after the socket is set, instead of being created in __init__ with None references and back-patched. __init__ just sets self._writer = None.
• glib: intentionally unchanged behavior. __init__ inlines the per-transport setup loop using the same shared helper. No _setup_socket method on the base class anymore.
• Docs: .. versionchanged:: v5.0.0 notes on aio.MessageBus.connect() and the class docstring. _create_socket_for_transport documents both InvalidAddressError and ValueError (for non-integer tcp:port=...).

Relationship to #592

#592 (async with bus context manager) is easier on top of this PR — __aenter__ no longer has to work around the constructor connecting the socket. dlech confirmed in #592 that this should land first.

Tests

• test_aio_connect_falls_back_between_transports — multi-transport address, all fail, last error raised.
• test_aio_connect_cleanup_after_socket_connected — auth failure after socket is connected clears all bus state.
• test_aio_connect_hello_error_propagates_and_clears_state — error reply to the initial Hello tears down sock/stream/fd/writer.
• test_create_socket_for_transport_{unix,tcp}_makefile_failure_closes_socket — verifies the makefile-failure cleanup path.
• test_glib_low_level gets a transport-fallback test covering the glib path.
• test_send_reply MockBus updated to populate _sock/_stream in connect() rather than __init__, matching the new pattern.

Fixes blocking-I/O detection errors like:

blockbuster.BlockingError: Blocking call to socket.socket.connect

Fixes: #569

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (e7b3941) to head (e1ff9e2).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #570      +/-   ##
==========================================
+ Coverage   89.90%   90.31%   +0.40%     
==========================================
  Files          29       29              
  Lines        3527     3561      +34     
  Branches      606      605       -1     
==========================================
+ Hits         3171     3216      +45     
+ Misses        211      204       -7     
+ Partials      145      141       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codspeed-hq]

Merging this PR will not alter performance

✅ 6 untouched benchmarks

---

_{Comparing agners:fix-sync-io-in-messagebus-init (e1ff9e2) with main (d59d7dc)}

[agners]

Probably most problematic is that Exceptions now get raised with connect(). But I'd guess that most users use the code with something like this:

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.

[dlech]

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 they 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.

[Copilot]

Pull request overview

Defers blocking socket connection work from BaseMessageBus.__init__() into connect() for the asyncio implementation, so MessageBus() construction no longer performs blocking I/O and connection errors surface from connect() instead.

Changes:

• Removed eager _setup_socket() call from BaseMessageBus.__init__() and introduced _create_socket_for_transport() to share per-transport socket construction.
• Updated aio.MessageBus.connect() to perform async connect attempts (including transport fallback) and updated docs to mark the breaking change.
• Adjusted GLib initialization to preserve prior behavior (explicit _setup_socket() in glib.MessageBus.__init__) and updated tests to expect failures from connect().

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/test_send_reply.py	Removes socket connect patching; uses a mock bus that pre-populates _sock/_stream now that init no longer connects.
tests/test_message_bus.py	Updates failure expectations to raise from connect() and asserts no socket/stream stored on failed connect.
src/dbus_fast/message_bus.py	Stops connecting in __init__; adds _create_socket_for_transport() and refactors _setup_socket() to use it.
src/dbus_fast/message_bus.pxd	Makes _setup_socket cpdef to keep it callable from Python subclasses.
src/dbus_fast/glib/message_bus.py	Explicitly calls _setup_socket() in __init__ to preserve existing blocking-connect semantics.
src/dbus_fast/aio/message_bus.py	Moves socket creation/connection into connect() using loop.sock_connect() and updates docstring/versionchanged notes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dlech]

I think this looks in pretty good shape now. Just a few more comments...

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bdraco]

@bluetoothbot review

[bluetoothbot]

PR Review — fix!: defer socket.connect() from init to connect()

Solid PR that addresses the underlying issue (blocking I/O in __init__) cleanly. Most reviewer feedback has been incorporated: _writer no longer back-patched, _create_socket_for_transport is now static, glib path inlined and intentionally left blocking, options parsed before socket allocation, makefile cleanup paths covered by tests, BREAKING-CHANGE clearly documented with versionchanged notes. The remaining concerns are non-blocking polish: (1) the outer cleanup block in connect() runs unprotected operations sequentially and could mask the original exception if any step raises; (2) failure during on_hello leaves _disconnected/_user_disconnect set asymmetrically from earlier-stage failures, which could surprise retry code; (3) ExitStack callback ordering closes the socket before the stream — functionally fine in CPython but reversed from the old enter_context order and from the prior code style; (4) the hello-error test polls a private dict instead of synchronizing on an event. None of these are merge blockers — once (1) and (3) are tightened up the PR is ready to land.

---

🟢 Suggestions

1. Cleanup operations are unprotected — one failure masks the original (`src/dbus_fast/aio/message_bus.py`, L333-341)

The cleanup block runs four operations sequentially with no per-step error handling. If self._loop.remove_reader(self._fd), self._stream.close(), or self._sock.close() raises during cleanup (e.g. event loop is closing on shutdown), the remaining cleanup is skipped and the original exception is replaced by the cleanup exception. Consider wrapping each in contextlib.suppress(Exception) so cleanup is best-effort and the original exception survives:

except BaseException:
    with contextlib.suppress(Exception):
        self._loop.remove_reader(self._fd)
    with contextlib.suppress(Exception):
        self._stream.close()
    with contextlib.suppress(Exception):
        self._sock.close()
    self._sock = self._stream = self._fd = self._writer = None
    raise

This matches the pattern already used in _finalize() at lines 550-557.

except BaseException:
    self._loop.remove_reader(self._fd)
    self._stream.close()
    self._sock.close()
    self._sock = None
    self._stream = None
    self._fd = None
    self._writer = None
    raise

2. ExitStack callback ordering closes socket before stream (reverse of the old enter_context order) (`src/dbus_fast/aio/message_bus.py`, L264-271)

The old _setup_socket used stack.enter_context(socket.socket(...)) then stack.enter_context(self._sock.makefile(...)), so on rollback the LIFO unwind closed the stream first, then the socket. The new code:

stack.callback(stream.close)
stack.callback(sock.close)

inserts stream.close first and sock.close second, so on rollback sock.close runs before stream.close — reversed from before.

CPython's socket._decref_socketios handles this correctly (closing the socket only flips _closed=True; the fd is freed when the makefile-owned ref also closes), so there is no fd leak. But the order is the opposite of every other place in the codebase and inconsistent with the glib path's own ordering (same bug in glib/message_bus.py:185-188). Swap the two callbacks, or use stack.enter_context(stream) / stack.enter_context(sock) to make the ordering self-evident.

stack.callback(stream.close)
stack.callback(sock.close)

---

Checklist

• No bare except / swallowed errors — suggestion ＃1
• Resource cleanup on error paths complete — suggestion ＃1
• State machine consistency across error paths — suggestion ＃2
• Cleanup ordering matches prior code — suggestion ＃3
• Test isolation / determinism — suggestion ＃4
• Breaking change documented
• Public API docs updated (versionchanged, raises)
• Test coverage for new branches (fallback, post-connect cleanup, hello error, makefile failure)
• No new blocking I/O on the event loop
• .pxd kept in sync (private method removed cleanly)

---

Summary

Solid PR that addresses the underlying issue (blocking I/O in __init__) cleanly. Most reviewer feedback has been incorporated: _writer no longer back-patched, _create_socket_for_transport is now static, glib path inlined and intentionally left blocking, options parsed before socket allocation, makefile cleanup paths covered by tests, BREAKING-CHANGE clearly documented with versionchanged notes. The remaining concerns are non-blocking polish: (1) the outer cleanup block in connect() runs unprotected operations sequentially and could mask the original exception if any step raises; (2) failure during on_hello leaves _disconnected/_user_disconnect set asymmetrically from earlier-stage failures, which could surprise retry code; (3) ExitStack callback ordering closes the socket before the stream — functionally fine in CPython but reversed from the old enter_context order and from the prior code style; (4) the hello-error test polls a private dict instead of synchronizing on an event. None of these are merge blockers — once (1) and (3) are tightened up the PR is ready to land.

---

Automated review by Kōancde3fd4
7ddb891
96f11a9
5fc616d
eb70732
03232eb
69d1072
319b5af
9640771
ff9b6e7
63ce07b
26dfbbf
be31c74
0faba1c
538833a
2223e04
37f5d1f
044bb18

[bdraco]

@bluetoothbot review

[bdraco]

I think its in good shape now but need to reconcile all the review comments

[agners]

I've went through all the review comments and double checked if they are addressed/addressed missing ones. From my point of view the PR is ready for another round of review.

[dlech]

I didn't have time to really go through the cleanup logic again, but given there are enough tests for it now, I think this is good now.

Just one comment on making tests more readable.

[bdraco]

Quick review on my laptop and the code looks right. Its a bit hard to follow so I want to do a deep 2 screen check on my desktop before approving. Than have AI check to make sure we didn't miss any review comments. I'll also ask the bot to do another pass.

[bdraco]

@bluetoothbot review

[bdraco]

Thanks @agners @dlech

[dlech]

Not sure why this isn't ip_port = int(options.get("port", 0)).