Skip to content

Better MultiDeviceTestContext workaround

This MR improves the guidance in the examples on how to workaround an issue with the MultiDeviceTestContext. The problem is that the MDTC runs in nodb mode, so tango.DeviceProxy needs to be patched to convert short-form FQDNs to long-form FQDNs. This requires us to know what port the MDTC is running on.

Previously, the advice was to bind to an available port on the local host, then release it, then tell the MDTC to use that port. This way, we know what port the MDTC is on, and can use that port in the patch.

def _get_open_port():
    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    s.bind(("", 0))
    s.listen(1)
    port = s.getsockname()[1]
    s.close()
    return port

HOST = get_host_ip()
PORT = _get_open_port()

_DeviceProxy = tango.DeviceProxy
mocker.patch(
    'tango.DeviceProxy',
    wraps=lambda fqdn, *args, **kwargs: _DeviceProxy(
        "tango://{0}:{1}/{2}#dbase=no".format(HOST, PORT, fqdn),
        *args,
        **kwargs
    )
)

with MultiDeviceTestContext(devices_info, host=HOST, port=PORT) as context:
    ....

However there is a potential race-condition here: something else could bind to the port in the time between when we release it and when the MDTC binds to it. This race condition would normally occur very rarely, but becomes common when running tests in parallel.

Actually, there is no need for any of this. The MDTC functions as both a context manager and a context. When we create the MDTC, it is a context manager. When we use the with notation on it, it is supposed to return a context, and in fact it returns self. Up until now we have tended to do both of these bits at the same time:

with MultiDeviceTestContext(devices_info) as context:
    ...

but actually we can split them:

context_manager = MultiDeviceTestContext(devices_info)
with context_manager as context:
    ...

Doing so allows us to set up the patch in between these two lines: after the MDTC (as context manager) has obtained a port been instantiated, but before it has returned a context (i.e. itself), and therefore before tango.DeviceProxy can possibly have been called. Better still, MDTC already has a public get_device_access method that performs the conversion from short-form to long-form for us. So the patch ends up being

context_manager = MultiDeviceTestContext(devices_info)
_DeviceProxy = tango.DeviceProxy
mocker.patch(
    'tango.DeviceProxy',
    wraps=lambda fqdn, *args, **kwargs: _DeviceProxy(
        context_manager.get_device_access(fqdn), *args, **kwargs
    )
)
with context_manager as context:
   ....

Much less code, and no race condition!


A tiny bonus tweak: arguments to the __exit__ method specify an exception (if any) that has occurred within the with block. The __exit__ method is supposed to return a boolean flag indicating whether that exception has been handled. In the case of MDTC.__exit__, it doesn't even look at its arguments, so obviously it should return False. Actually it does not return anything, which means it implicitly returns None, which turns out to be just as good. However it is more readable to explicitly return False here. Also, if nothing is explicitly returned, then clients that try to read the return value will fall afoul of pylint's assignment-from-no-return error. Therefore I have added an explicit return False.

Edited by Drew Devereux

Merge request reports