search

epics-base / pvAccessCPP

pvAccessCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvAccessCPP/
Other
10 stars 22 forks source link

Resource leak in Beacon tracking #184

Open mdavidsaver opened 2 years ago

mdavidsaver commented 2 years ago

The m_beaconHandlers map can grow without bound as new PVA servers come online.

https://github.com/epics-base/pvAccessCPP/blob/b2f0aecfa362a0616758d7aefe3006c21abe754d/src/remoteClient/clientContextImpl.cpp#L4525-L4530

This map needs to be bounded in size and/or time. (like PVXS does...)

mdavidsaver commented 2 years ago

This may have manifest first with RTEMS because BeaconHandler::_mutex is a far more finite resource on that target.

https://epics.anl.gov/tech-talk/2022/msg00597.php

mdavidsaver commented 2 years ago

I remember noticing this before, and also remember fixing it. But apparently I'm getting confused with a similar leak in code tracking TCP connections. cf. https://github.com/epics-base/pvAccessCPP/commit/f1defe4e9ff3241226a0cee76d3ff7212b73fa48 fixed in 6.1.0

kasemir commented 2 years ago

What if the client simply ignores beacons? For existing but idle connections, there's already the periodic 'echo' mechanism. For disconnected channels, the search settles to once every ~30 seconds. Detecting beacons from new servers could reduce time to re-connect from those 30 seconds, but after a long disconnect another 30 seconds aren't too bad. So the beacons aren't absolutely necessary, and can in fact cause downtime, as we experienced with CA when clients woke up because of ill-timed beacons, resulting in a lot of search traffic, https://github.com/epics-base/jca/pull/53 .

mdavidsaver commented 2 years ago

What if the client simply ignores beacons?

As I understand things, the only ill effect should be slower reconnects. I usually describe Beacons as an optimization.