Fix `*_asynch` methods on DeviceProxy
Closes #590 (closed)
Investigating this ticket, I found quite a lot of issues with other *_asynch
methods. Some comments:
-
read_attribute_asynch
withPUSH_CALLBACK
can be fixed in two ways. First I suggested cppTango!1224 (closed) but Thomas Barun said that probably we want to have it like it is in CppTango, so pure PyTango fix is 1f6083c5 -
write_attribute_asynch
was not working completely for a different reason. Callback function passed to this function is being wrapped inPyCallbackAutoDie
class object here and CppTango expectsattr_written
method to be present, notattr_write
like it was. Fixed with 4509db2d - In case callback function is called properly, but we
raise Exception
inside it, we have another problem. In boost we were deleting event object, but because it is already managed byunique_ptr
here when theConnection::Cb_ReadAttr_Request
functions ends andunique_ptr
gets out of scope I got adouble free detected in tcache 2
crash. So I suggest 5fb68b04 but somebody more experienced can comment on this. - Exceptions raised inside callback function are not handled at all in CppTango and they crash the client, even if it's written in Cpp. If this a expected behaviour can be discussed, but in case of python exceptions we don't even get the exception message. I think we can't handle it in PyTango, because the callback functions are ran in thread completely created in Cpp layer here and only
omni_thread_fatal
exceptions are handled here. Maybe we should at least add something like this 37bc8f1d to log the error messages? - Another thing I found is inconsistency in how
read_attribute_asynch
behaves when attribute timeout and when it raises an Exception. I would expect in both cases a callback to be informed withev.err=True
. But it doesnt happen. Exception raised in attribute still makes this function enter this if like nothing wrong happened andev.err
is set toFalse
andev.argout
is a list of attributes withhas_failed
set toTrue
. This is the only way we can tell something wrong happened, but if we usewrite_attribute_asynch
function, the problem is the same, but we don't getev.argout
so we can't even tell if the write was successfull or not. I suspect this to be a bug of CppTango and I opened an issue here cppTango#1231 (closed) For now I wrote PyTango tests so they pass with this inconsistency and I left the comments - Last issue is that the asynch tests seem to behave differently depending on if we use
process=True
or not. I think it should not be like that and the behaviour should be consistent, but I didn't have time to investigate the cause. You can removeprocess=True
to see how the result change.
Notes:
Details of `read_attribute_asynch` problem
device_proxy.read_attribute_asynch("attr", callback)
was not working, failing with segfault. The reason was ev->argout
was a not initialized pointer nullptr
with value 0x0
.
Function Connection::Cb_ReadAttr_Request
of cppTango here (that is responsible for running the callback) initializes dev_attr
pointer (later converted to ev->argout
) conditionally. If theres no exception, it's std::vector<DeviceAttribute>
. But if there was omni::TRANSIENT_CallTimedout
exception, it stays nullptr
.
It was being passed from cpptango here to PyCallBackAutoDie::attr_read
here to here and to PyDeviceAttribute::convert_to_python
in PyTango boost. And finally this line tried to dereference it with ->empty()
and it crashed.
The solution was to either allways initialize this pointer with a vector in CppTango, even if the vector stays empty, then callback would receive []
or to handle nullptr
in PyTango and convert it to None
in convert_to_python
.
I considered if the second option is OK. convert_to_python
function is used also in files: callback.cpp
(in PyCallBackPushEvent::fill_py_event
but we create a new Tango::DeviceAttribute
there, so it's not nullptr
anyway) and in device_proxy.cpp
(where we get the response from read_attribute
so we don't expect a nullptr
anyway). So the change seems safe. The only risk
is that if CppTango returns nullptr
for some reason, we will not crash PyTango, but continue with None
instead. We could consider adding similar change to boost::python::object convert_to_python(TDeviceAttribute* dev_attr, Tango::DeviceProxy & dev_proxy, PyTango::ExtractAs extract_as)
signature, but I think theres no risk nullptr
will be passed there anyway.