Skip to content

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 with PUSH_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 in PyCallbackAutoDie class object here and CppTango expects attr_written method to be present, not attr_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 by unique_ptr here when the Connection::Cb_ReadAttr_Request functions ends and unique_ptr gets out of scope I got a double 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 with ev.err=True. But it doesnt happen. Exception raised in attribute still makes this function enter this if like nothing wrong happened and ev.err is set to False and ev.argout is a list of attributes with has_failed set to True. This is the only way we can tell something wrong happened, but if we use write_attribute_asynch function, the problem is the same, but we don't get ev.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 remove process=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.

Edited by Mateusz C

Merge request reports

Loading