Skip to content

Rename BaseClient.disconnect() to close() to avoid collision with QObject.disconnect() #126

@marcosfrenkel

Description

@marcosfrenkel

Problem

BaseClient.disconnect() (added in 9f9145b, 2020-05-28) shares a name with Qt's QObject.disconnect(signal, slot), which is used to unwire signal/slot connections. This collision bites anywhere a client subclass also inherits from QObject — currently QtClient and its subclasses (EmbeddedClient, etc.).

Because of Python's MRO (class QtClient(_QtAdapter, Client)), a plain qt_client.disconnect() call resolves to QObject.disconnect first, silently skipping the zmq socket/context teardown in BaseClient.disconnect. This left dangling zmq resources during test teardown and process exit.

We worked around it in src/instrumentserver/client/proxy.py with an override that dispatches by argument presence:

def disconnect(self, *args, **kwargs):
    # QObject.disconnect() shadows BaseClient.disconnect() via MRO, so
    # explicitly dispatch to BaseClient.disconnect() when called without
    # Qt signal arguments. Preserve Qt's signal-disconnect semantics when
    # invoked with signal arguments (e.g. disconnect(signal, slot)).
    if not args and not kwargs:
        return Client.disconnect(self)
    return _QtAdapter.disconnect(self, *args, **kwargs)

This works but is a smell — every future QtClient-based class has to carry (or risk forgetting) the workaround.

Proposed fix

Rename BaseClient.disconnect()BaseClient.close().

  • No collision with QObject.disconnect, so the override in QtClient can be removed.
  • Matches Python convention for resource teardown (files, sockets, DB connections, context managers).
  • BaseClient.__exit__ becomes a one-liner calling self.close().

Backwards compatibility

disconnect() has been a public API for ~5 years, so external users likely depend on it. Keep disconnect() as a thin deprecated alias:

def disconnect(self):
    warnings.warn("BaseClient.disconnect() is deprecated, use close() instead",
                  DeprecationWarning, stacklevel=2)
    self.close()

Then the QtClient.disconnect override is still needed for the alias but can be removed in a future major version once the alias is dropped. Alternatively, drop the alias immediately and call it a breaking change in the next release notes.

Call sites to update (internal)

  • src/instrumentserver/client/core.py__exit__, the timeout-reconnect path in ask(), cleanup in connect()
  • src/instrumentserver/client/proxy.pyClientStation.disconnect (likely also rename to .close() for consistency), QtClient.disconnect override can be removed
  • src/instrumentserver/server/application.pyServerGui.closeEvent
  • test/pytest/conftest.pycli fixture teardown

Context

Discovered while hardening ZMQ/Qt teardown for the pytest suite on the modernizing_packaging branch (commit 6de50fe).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions