prior: fix AttributeError for bad connections

[xtofl]

When constructing a ProScanIII object on a port that does not connect to a Prior controller, the _devices member is not defined.

This causes the __del__ operation of the object to fail, because it will end up in the base class' __del__, which requires the devices property, which is overridden to use the self._devices member.

Conceptually, the base class requires the implementation to be a valid object. One may also argue that the object is not allowed to be constructed at all if not connected to a proper device; that would be an alternative fix.

I tested (Before/After) with this snippet:

from microscope.controllers import prior

good = prior.ProScanIII("COM5")
try:
    bad = prior.ProScanIII("COM3")
except RuntimeError as e:
    assert "Not a ProScanIII device" in str(e)

[carandraug]

Thank you for reporting, debugging, and fixing this.

I just need a clarification: would it suffice to swap the lines declaring the _devices and _conn attribute? From your explanation I guess that would suffice but this commit makes other small changes and I don't know if they are also needed.

[xtofl]

Great remark! You are right: I'll be happy to redo the commit to only contain the essential changes.

[xtofl]

Wait - no: the essence is that the the object is in a 'valid' state always, i.e. before any exception can be thrown, the _devices member has to be set.

[xtofl]

I went ahead and simplified the change.

[carandraug]

Thank you. That is clear. I've been thinking about this issue on the rest of the project and how could have prevented this from happening (and how we ensure it doesn't happen in other devices).

What I'm thinking is that the root issue is that shutdown is being called when the object has not yet been fully initialised. Even if you move the declaration of _devices to the top of __init__ 9as you just did) you still may get an exception happen before it. See:

>>> from microscope.controllers.prior import ProScanIII
>>> x = ProScanIII()
Exception ignored in: <function Device.__del__ at 0x7f4f656bbc40>
Traceback (most recent call last):
  File "/home/carandraug/src/python-microscope/microscope/microscope/abc.py", line 291, in __del__
    self.shutdown()
  File "/home/carandraug/src/python-microscope/microscope/microscope/abc.py", line 394, in shutdown
    self._do_shutdown()
  File "/home/carandraug/src/python-microscope/microscope/microscope/abc.py", line 1326, in _do_shutdown
    for d in self.devices.values():
             ^^^^^^^^^^^^
  File "/home/carandraug/src/python-microscope/microscope/microscope/controllers/prior.py", line 226, in devices
    return self._devices
           ^^^^^^^^^^^^^
AttributeError: 'ProScanIII' object has no attribute '_devices'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ProScanIII.__init__() missing 1 required positional argument: 'port'

You mention on the initial comment "One may also argue that the object is not allowed to be constructed at all if not connected to a proper device; that would be an alternative fix.". Can you expand on that, please?

[xtofl]

Yes sure. That was a more theoretical idea, and it doesn't directly map onto Python, but onto languages like C++, where class hierarchy construction/destruction order is determined by the language.

If we take step back from the design here, we can see an interaction between the general device management logic and the specific details for each device. This relation has been established via inheritance, which makes 'general' functions depend on 'specific' details to be completely initialized.

The choice for inheritance throws in an extra complication: the language runtime will call some 'general' functions (like __del__) at times we don't expect it.

This is where another design (aggregation i.s.o. inheritance) would have avoided this possibility:

class Controller:
  def __init__(self, specifics):
    self._specifics = specifics
  def shutdown(self):
   for d in self._specifics.devices:
     d.shutdown()

class Prior:
  def __init__(self, port):
    if not Prior.handshake(port):
      raise RuntimeError

bad_prior = Prior("wrong-port")  # would raise here
controller = Controller(bad_prior)  # would not be reached => no possible interaction

[xtofl]

What I'm thinking is that the root issue is that shutdown is being called when the object has not yet been fully initialised. Even if you move the declaration of _devices to the top of __init__ 9as you just did) you still may get an exception happen before it. See:

...
> TypeError: ProScanIII.__init__() missing 1 required positional argument: 'port'

That is what I was referring to: if an object is not initialized, it is not valid for use. As such, any exception raised in __init__ should disallow any other methods (and __del__) to rely on anything. I just learned, by the way, that __del__ is the counterpart of __new__, and is not even guaranteed to be called (cf. object.del?

So maybe the root cause is that the __del__ method does deinitialization, and a more generic solution is possible. This may incur a breaking change if end-users rely on __del__ shutting down their devices.

I like to refer to exception safety concepts in C++, e.g. at microsoft or cppreference.

[carandraug]

I see your point about this not being a problem if we were using C++. But this is a Python project for a reason :) Using C++ would bring its own set of problems particularly when considering the target audience of this library and the ecosystem we want to interact with. Also, note that __init__ is the initializer and not the object constructor (that is __new__) so mapping concepts from C++ to Python is not so clear.

With regards to aggregation vs inheritance, I don't think that'd be the correct design since this controller "is a" device. It doesn't "have a" device. Although if the class was a device manager, then it could be said to "have a" device.

---

Would you agree with me that the root cause of this is that when __del__ is called we don't know the state of the device? And if so, a better solution would be to check on __del__ ? Or maybe __del__ shouldn't attempt a "nice shutdown" and that should only be done when the user explicitly calls it?

(to be honest with your PR, I'm not sure about moving super.__init__() until after the subclass has done its work. I see the pros of it - and can imagine some cons as well - but if we're doing that change I'd rather see it across the project for consistency).