Make `bool(CANothing)` always return True

[coretl]

https://github.com/dls-controls/aioca/blob/12a0a68c249b90cd375b9432ed438c7a7e85ab97/aioca/_catools.py#L92-L93

Causes issues with various python internals, like concurrent.futures.result which does: https://github.com/python/cpython/blob/d5a8d4b19670b930cd6cb5e18e267877ebe49233/Lib/concurrent/futures/_base.py#L399

So if we put a CANothing in as a result it shows as no exception...

[coretl]

Suggestion:

• Make this change in aioca
• But need to make sure we don't accidentally cause bugs when going from cothread -> aioca
• So make ca_nothing.__bool__ emit a one time warning in cothread, telling the user to use ca_nothing.ok instead

[coretl]

Can't make the warning in aioca as this will be too noisy because of the concurrent.futures.Future usage above

[Araneidae]

This issue is exasperating, because it arises from a combination of two mistakes:

1. It looks as if adding a "truthiness" test to ca_nothing was a mistake. Certainly this test is not necessary as all CA values returned by cothread support the .ok attribute.
2. Bluntly, the tests in concurrent.futures._base are wrong, and should have been written as if self._exception is not None. However, this appears to be established practice so we have to adapt our libraries accordingly.

I have raised https://github.com/DiamondLightSource/cothread/issues/67 against https://github.com/DiamondLightSource/cothread

[Araneidae]

Unfortunately we cannot just delete the implementation of __bool__. Just as for cothread, the direct testing of the ca_nothing value returned from caput is recommended in the documentation: https://github.com/dls-controls/aioca/blob/12a0a68c249b90cd375b9432ed438c7a7e85ab97/docs/api.rst?plain=1#L54-L59

I don't know what a suitable fix is.

[Araneidae]

@coretl , can you please expand on the underlying issue here? Are you catching ca_nothing exceptions somewhere and then hoping to re-raise them? If so I think maybe a cleaner fix might be to split the ca_nothing class into two classes, one of which is a true Exception object (which is what is raised on failures), and the other is not.

In a little more detail with "bikeshed" names:

class ca_nothing_exception(Exception):
    ...
class ca_nothing:
    ...
    def __bool__(self): return self.ok

and when maybe_throw catches a ca_nothing_exception value it converts it into a ca_nothing value.

Am not sure what this would break.

[coretl]

Here's another option, we give CANothing.__init__ a bool_checks_ok option that is only set to True when throw=False. This means that the docs are still valid, but any exception raised by throw=True will always have bool(exception) == True.

That covers the majority use case of truthiness with throw=False (as highlighted by docs) as well as the Future use case of throw=True

[coretl]

Are you catching ca_nothing exceptions somewhere and then hoping to re-raise them?

Yes, this is what the Task/Future interface in asyncio is doing. The co-routine calls caput with the implicit throw=True, then the asyncio code catches it and stuffs the value into a Future.

If so I think maybe a cleaner fix might be to split the ca_nothing class into two classes, one of which is a true Exception object (which is what is raised on failures), and the other is not.

Snap! Yes, that makes sense. I think your solution of splitting into 2 classes rather than mine of an __init__ variable is better.

So throw=True raises a CANothingException without a __bool__ and throw=False returns a CANothing with a __bool__.

[Araneidae]

Yes, and I think this should be easy enough to implement. Unfortunately I think we're stuck with the ca_nothing name for values, and changing this name to a PEP compliant name is a separate patch. However we could add alias names for now.

Here is my first thought for a patch against cothread; I imagine an aioca patch would be similar:

diff --git a/src/cothread/catools.py b/src/cothread/catools.py
index a209829..069a062 100644
--- a/src/cothread/catools.py
+++ b/src/cothread/catools.py
@@ -83,10 +83,9 @@ K = 1024
 CA_ACTION_STACK         = _check_env('CATOOLS_ACTION_STACK', 0)

-class ca_nothing(Exception):
+class ca_nothing:
     '''This value is returned as a success or failure indicator from caput,
-    as a failure indicator from caget, and may be raised as an exception to
-    report a data error on caget or caput with wait.'''
+    or as a failure indicator from caget.'''

     def __init__(self, name, errorcode = cadef.ECA_NORMAL):
         '''Initialise with PV name and associated errorcode.'''
@@ -102,13 +101,34 @@ class ca_nothing(Exception):

     def __bool__(self):
         return self.ok
-    __nonzero__ = __bool__   # For python 2
+
+# Naming convention compatible alias
+CANothing = ca_nothing
+
+
+class ca_nothing_exception(Exception):
+    '''This exception may be raised as an exception to report a data error on
+    caget or caput with wait.'''
+
+    def __init__(self, name, errorcode = cadef.ECA_NORMAL):
+        '''Initialise with PV name and associated errorcode.'''
+        self.ok = errorcode == cadef.ECA_NORMAL
+        self.name = name
+        self.errorcode = errorcode
+
+    def __repr__(self):
+        return 'ca_nothing_exception(%r, %d)' % (self.name, self.errorcode)
+
+    def __str__(self):
+        return '%s: %s' % (self.name, cadef.ca_message(self.errorcode))

     def __iter__(self):
         '''This is *not* supposed to be an iterable object, but the base class
         appears to have a different opinion.  So enforce this.'''
         raise TypeError('iteration over non-sequence')

+CANothingException = ca_nothing_exception
+

 def maybe_throw(function):
     '''Function decorator for optionally catching exceptions.  Exceptions
@@ -125,8 +145,8 @@ def maybe_throw(function):
             # will be raised anyway, which seems fair enough!
             try:
                 return function(pv, *args, **kargs)
-            except ca_nothing as error:
-                return error
+            except ca_nothing_exception as error:
+                return ca_nothing(error.name, error.errorcode)
             except cadef.CAException as error:
                 return ca_nothing(pv, error.status)
             except cadef.Disconnected:
@@ -145,7 +165,7 @@ def ca_timeout(event, timeout, name):
     try:
         return event.Wait(timeout)
     except cothread.Timedout as timeout:
-        raise ca_nothing(name, cadef.ECA_TIMEOUT) from timeout
+        raise ca_nothing_exception(name, cadef.ECA_TIMEOUT) from timeout

 # ----------------------------------------------------------------------------
@@ -593,7 +613,8 @@ def _caget_event_handler(args):
         else:
             cothread.Callback(done.Signal, value)
     else:
-        cothread.Callback(done.SignalException, ca_nothing(pv, args.status))
+        cothread.Callback(
+            done.SignalException, ca_nothing_exception(pv, args.status))

 @maybe_throw
@@ -772,7 +793,8 @@ def _caput_event_handler(args):
         if args.status == cadef.ECA_NORMAL:
             cothread.Callback(done.Signal)
         else:
-            cothread.Callback(done.SignalException, ca_nothing(pv, args.status))
+            cothread.Callback(
+                done.SignalException, ca_nothing_exception(pv, args.status))
     if callback is not None:
         cothread.Callback(callback, ca_nothing(pv, args.status))

Exceptions are raised in fewer places than I was expecting.