Pushing events with a value for state and status triggers a memory leak

I noticed a two issues with the pushing of events with values for state and status:

• values, stamps and qualities are ignored
• a memory leak is triggered

It can be reproduced with the following code:

#! /usr/bin/python
from tango import DevState, AttrQuality
from tango.server import Device, command

class TestState(Device):

    def init_device(self):
        self.set_state(DevState.OFF)
        self.set_change_event('State', True, False)

    @command(polling_period=100)
    def update(self):
        self.push_change_event(
            'State', DevState.OFF, 1.0, AttrQuality.ATTR_VALID)

if __name__ == '__main__':
    TestState.run_server()

Created by: grm84

I have also seen this problem pushing state, which I have tracked down, with valgrind, to code in attrSetVal.cpp

==1555== 504 (432 direct, 72 indirect) bytes in 18 blocks are definitely lost in loss record 9,586 of 10,724
==1555==    at 0x4C29203: operator new(unsigned long) (vg_replace_malloc.c:334)
==1555==    by 0xF54D61E: Tango::Attribute::set_value(Tango::DevState*, long, long, bool) (attrsetval.cpp:1460)
==1555==    by 0xEAB3BCC: __set_value_scalar<19l> (attribute.cpp:86)
==1555==    by 0xEAB3BCC: PyAttribute::__set_value(std::string const&, Tango::Attribute&, boost::python::api::object&, long*, long*, double, Tango::AttrQuality*) (attribute.cpp:322)
==1555==    by 0xEAB4350: PyAttribute::set_value(Tango::Attribute&, boost::python::api::object&) (attribute.cpp:348)
==1555==    by 0xEB20AB7: PyDeviceImpl::push_change_event(Tango::DeviceImpl&, boost::python::str&, boost::python::api::object&) (device_impl.cpp:229)

State and Status are special attributes because they existed even before the attributes were invented in Tango. They were only commands before. Now they are commands and attributes. They are also special because they are the only attributes which must be present in every Tango device class. So they are treated differently in the code. By looking at attribute.tpp:1371 and attribute.cpp:3162, one can see the State and Status quality is forced to ATTR_VALID when an event is pushed (These methods are invoked by the fire_xxx_event() methods) so the way it is currently implemented, no matter what quality factor is passed to push_xxx_event() methods, the quality factor in the event will always be valid for State and Status attributes. This might be intentional since they are also commands, and commands do not have quality factors....

While investigating on this issue, I also noticed the DevState (resp. DevString) value passed to push_change_event() is actually ignored for State (resp. Status) attribute. The value taken into account is the current state (resp. status) of the device which you can get with get_state() (resp. get_status()) method. The timestamp passed to push_xxx_event() method is also ignored with state and status and the current date and time seems to be used instead.

About the memory leak, this leak is due to the fact that when invoking push_change_event(), the DevState value given by the user is stored into a sequence when push_change_event() invokes Attribute::set_value(). This sequence is not deleted as it should in fire_change_event() where there is a nice if, which makes the code refusing to delete the sequence when the attribute name is State or Status. See attribute.cpp:3839 for instance. This if in fire_change_event() is suspicious and seems to suggest that the sequence should never have been allocated for State and Status attributes, so this means that Attribute::set_value() should never be invoked by push_xxx_event() if the attribute name is state or status... It looks like this was the intended philosophy behind the current implementation (to be confirmed), even tough this does not seem to be documented. I guess this was done like that to ensure that the State and Status events sent are matching what a client could see if he would invoke the State or Status command. The problem is that it is not documented, confusing and also I think it does not make sense to force the timestamps to the current date and time when pushing state and status events. And I think we should be able to choose the quality factor too.

@bourtemb , thanks again for very detailed analysis of the issue!!!

mentioned in merge request !504

changed the description

changed milestone to %9.3.5

changed milestone to %9.3.6

removed time estimate

changed milestone to %9.5.0

unassigned @Ingvord

changed milestone to %10.0.0

added Bug label

changed milestone to %10.1.0

@yamatveyev , do you think !1303 (closed) could also solve this issue?

I have to look, but almost sure taht yes, it will help

It seems I was wrong - no !1303 (closed) won't help here, since in fire_<>_event there is everywhere such code:

            if((name_lower != "state") && (name_lower != "status"))
            {
                // delete the data values allocated in the attribute
                bool data_flag = get_value_flag();
                if(data_flag)
                {
                    if(quality != Tango::ATTR_INVALID)
                    {
                        delete_seq();
                    }
                    //                        set_value_flag (false);
                }
            }

So we do not clear value if we pushing event for state and status....

added Discussion needed label

We have a test mechanism in PyTango that could check this. Currently, it is disabled, but if it gets fixed here, we can enable it.

https://gitlab.com/tango-controls/pytango/-/blob/v10.0.0/tests/valgrind/memory_test.py?ref_type=tags#L304-308

assigned to @thomasives and unassigned @bourtemb

removed Discussion needed label

I have tried reproducing this with PyTango 10.0.0 with Python 3.12 and I do not see a memory leak associated with a push_change_event in valgrind when I run the above device server. I do see a leak if I run this under PyTango 9.5.0:

==401486== 560 (480 direct, 80 indirect) bytes in 20 blocks are definitely lost in loss record 23,162 of 25,363
==401486==    at 0x4843FEC: operator new(unsigned long) (vg_replace_malloc.c:487)
==401486==    by 0x136EDCE4: void Tango::Attribute::set_value<Tango::DevState, (Tango::DevState*)0>(Tango::DevState*, long, long, bool) (in /home/tom/osl/tango/scratch/venv/lib/python3.12/site-packages/tango/_tango.so)
==401486==    by 0x13733350: PyAttribute::__set_value(std::string const&, Tango::Attribute&, boost::python::api::object&, long*, long*, double, Tango::AttrQuality*) (in /home/tom/osl/tango/scratch/venv/lib/python3.12/site-packages/tango/_tango.so)
==401486==    by 0x13734187: PyAttribute::set_value_date_quality(Tango::Attribute&, boost::python::api::object&, double, Tango::AttrQuality) (in /home/tom/osl/tango/scratch/venv/lib/python3.12/site-packages/tango/_tango.so)
==401486==    by 0x1368E472: PyDeviceImpl::push_change_event(Tango::DeviceImpl&, boost::python::str&, boost::python::api::object&, double, Tango::AttrQuality) (in /home/tom/osl/tango/scratch/venv/lib/python3.12/site-packages/tango/_tango.so)

I think this means the issue has been fixed. But I cannot workout what change we did that actually fixed this. @yamatveyev Do you have any ideas?

To me it still looks like we are not calling delete_seq for the state or status attributes: https://gitlab.com/tango-controls/cppTango/-/blob/84fc51b24b2910d7a12487930332812012fec206/src/server/attribute.cpp#L2970-2972

assigned to @yamatveyev and unassigned @thomasives

created branch 368-pushing-events-with-a-value-for-state-and-status-triggers-a-memory-leak to address this issue

mentioned in merge request !1349

added memory leaks label