Skip to content

feat: context manager protocol#592

Open
xi wants to merge 1 commit intoBluetooth-Devices:mainfrom
xi:context-manager
Open

feat: context manager protocol#592
xi wants to merge 1 commit intoBluetooth-Devices:mainfrom
xi:context-manager

Conversation

@xi
Copy link

@xi xi commented Feb 12, 2026

This makes it easy to disconnect the bus once you are done.

@codecov
Copy link

codecov bot commented Feb 12, 2026

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.62%. Comparing base (5c61e6f) to head (b843e94).

Files with missing lines Patch % Lines
src/dbus_fast/aio/message_bus.py 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
- Coverage   85.66%   85.62%   -0.04%     
==========================================
  Files          29       29              
  Lines        3480     3493      +13     
  Branches      601      601              
==========================================
+ Hits         2981     2991      +10     
- Misses        308      311       +3     
  Partials      191      191              

☔ 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
Copy link

codspeed-hq bot commented Feb 12, 2026

Merging this PR will not alter performance

✅ 6 untouched benchmarks


Comparing xi:context-manager (b843e94) with main (5c61e6f)

Open in CodSpeed

@xi xi force-pushed the context-manager branch 2 times, most recently from 107224c to 9302e0d Compare February 12, 2026 09:06
Copy link
Member

@dlech dlech left a comment

Choose a reason for hiding this comment

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

Unfortunately, currently this is not so simple. This is due to how sockets and connections are currently handled, which is not ideal.

The problem is that the socket is actually connected in the constructor (__init__()) instead being done in the connect() method. So disconnect() needs to be called always even if connect() fails. Otherwise we would leak resources.

I suppose we could add a try/except in the __enter__() method to call disconnect() if connect() fails. But ideally, we would address #570 first to fix that situation.

This makes it easy to disconnect the bus once you are done.
@xi xi force-pushed the context-manager branch from 9302e0d to b843e94 Compare February 13, 2026 15:00
@xi
Copy link
Author

xi commented Feb 13, 2026

Thanks for the feedback. If I understand correctly, with your changes this is already an improvement even if #569 would not get fixed.

@dlech
Copy link
Member

dlech commented Feb 13, 2026

Yes, I think we shouldn't be blocked by #569.

And we should add a .. versionchanged:: to the documentation for this and add a test that covers the case when connect() fails.

Then I think we could merge it.

@xi
Copy link
Author

xi commented Feb 13, 2026

and add a test that covers the case when connect() fails.

I had a quick look at this but it wasn't obvious how connect() should fail. I looked for examples of something similar in the existing tests, but there was nothing obvious to me. Any pointers?

@dlech
Copy link
Member

dlech commented Feb 13, 2026

Two ideas:

  1. Make a socket that isn't a D-Bus socket. For example using pipe() to create a socket pair. The idea is that socket.connect() should work but when we actually try to authenticate it will raise an exception
  2. If we can't figure out something with a real socket, we could mock it. I would consider this a last resort though.

@xi
Copy link
Author

xi commented Feb 15, 2026

I am sorry, I am really stuck here. I tried something like this, but connect() just got stuck instead of raising an exception:

@pytest.mark.asyncio
async def test_context_manager_failing():
    host = "127.0.0.1"
    port = "55556"

    async def handle_connection(reader, writer):
        writer.write(b'bogus\r\n')
        await writer.drain()
        writer.close()
        await writer.wait_closed()

    server = await asyncio.start_server(handle_connection, host, port)
    bus = MessageBus(bus_address=f"tcp:host={host},port={port}")

    assert not bus.connected
    async with bus:
        assert bus.connected

    assert not bus.connected
    server.close()

Mocking doesn't work either because AttributeError: 'MessageBus' object attribute 'connect' is read-only

@dlech
Copy link
Member

dlech commented Feb 16, 2026

I think you are on the right track. I like the idea of making a "naughty" TCP server. 😄

I would suggest to make the server actually wait for a request and then respond to it. Something like:

@pytest.mark.asyncio
async def test_context_manager_failing():
    host = "127.0.0.1"
    port = "55556"

    tasks: list[asyncio.Task[Any]] = []

    async def handle_connection(reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> None:
        async def read() -> None:
            while True:
                try:
                    data = await reader.read(100)
                    if not data:
                        break

                    writer.write(b"this is not the correct response\r\n")
                    await writer.drain()
                except OSError:
                    break

        tasks.append(asyncio.create_task(read()))

    server = await asyncio.start_server(handle_connection, host, port)
    bus = MessageBus(bus_address=f"tcp:host={host},port={port}")

    assert not bus.connected

    with pytest.raises(ValueError, match="not a valid _AuthResponse"):
        async with bus:
            pass

    assert not bus.connected

    server.close()

(This needs a bit more work to cancel the reader/writer task and close the reader/writer.)

Then, I think this test exposes a bug in my suggestion. It looks like it is hanging on await self.wait_for_disconnect().

This is really a deeper problem in dbus-fast in that this can hang under certain conditions because it requires I/O to be attempted in order to trigger an exception in a reader or writer callback that in turn triggers the finalizer that the method is waiting on.

So we probably need to just call self._finalize() instead.

    async def __aenter__(self) -> MessageBus:
        try:
            return await self.connect()
        except BaseException:
            self._finalize()
            raise

await self.wait_for_disconnect()
raise

async def __aexit__(self, *args, **kwargs) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
async def __aexit__(self, *args, **kwargs) -> None:
async def __aexit__(self, exc_type: type[BaseException] | None, exc_value: BaseException | None, traceback: TracebackType | None, /) -> None:

Let's use the proper type hints here.

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

Comments