Experimental Physics and
Industrial Control System

Michael Davidsaver via Tech-talk <tech-talk at aps.anl.gov> · Sat, 23 Jul 2022 12:19:41 -0700

On 7/21/22 05:15, Jure Varlec wrote:

Don't get me wrong, I don't disagree with you. Your suggested approach
definitely crosses all the ts and dots all the is. I just wanted to know whether
there's a problem with simply disabling the port that you are aware of and I'm
not.

No, no specific problem based on what has been said.

I'm sure you understand that I, and possibly Érico and others who have
existing drivers with existing cleanup code, have a judgment call to make. Your
suggested approach may be Best Practice in this context, but in general, so is
RAII, and they are not quite aligned.

I'm not sure "in general" is valid in general :)

I think you may be misinterpreting my "not best practice" as
"never do this", which is not my intent.

Rather, I'm trying to make clear that in this case there is
no one right answer.  Either a asynPortDriver object will
be "leaked", or there will be accessible dangling pointers to
it.  So a trade off will need to be made.

... or asyn could be modified to allow these dangling pointers
to be made permanently inaccessible.  This is OSS after all.

Based on my past (mis)adventures in C, I have a strong
personal preference to avoid dangling pointers.

What you're proposing requires changing
the design of the driver such that it can be made defunct and resources released
without actually destroying it. And doing a redesign of a working driver has to
be weighed against simply disabling the port before destroying it, which seems
to work well in practice.

Disabling the port is a good first step.

What I'm suggesting may be no more than moving the current
contents of a sub-class destructor over to a new
epicsAtExit callback.

This callback would then: 1. disable the asyn port,
2. interrupt the driver worker thread, and 3. join the worker
thread.  It would then return _without_ deleting the object.

This way, if there is something more going on now, or in future.
eg. if the asyn port somehow gets re-enabled, or if some
asyn client "cheats".  Then the likely consequences would either
be nothing, when reading a stored parameter.  Or a deadlock, when
waiting for a now defuct worker thread to do something.

In my personal taxonomy of pain, a finding and fixing a deadlock
is preferable to memory corruption.

Another thing I'd like to point out is that your proposal moves the
responsibility of preventing the read and write calls to each driver.

At present, I don't see an easy alternative.  (there are
not easy alternatives.  see below...)

Which
works, of course. But I find it much more elegant to have these checks further
up the stack. Which, for the case of IOC scan threads which go through asyn
device support that, in turn, calls asynManager's queueRequest method, is
already the case! Don't you think it would make sense to enshrine (and perhaps
improve) this behavior and make life simpler for the drivers? I mean, doing
proper clean up in EPICS is fraught,

Yes, absolutely there is room for improvement.  And I've
been (very slowly) working towards a goal of allowing
IOCs to shutdown and cleanup.

The complications are always compatibility and legacy code...

eg. One concern with stopping the scan threads is that
some device support calls epicsExit(), which would deadlock.

To address this I introduced epicsExitLater(), and changed
iocStats to use it.

https://github.com/epics-modules/iocStats/blob/70128c77ad97529726adc0c3c590241067e7db9f/devIocStats/os/default/devIocStatsOSD.h#L29-L33

More recently I've introduced what I've referred to as
de-init hooks for better symmetry with actions taken
during iocInit().  (and which facilitate unit testing
of device support)

https://github.com/epics-base/epics-base/blob/d82ab819ef751f5c1f0c7e4427cbef466fc4629e/modules/libcom/src/iocsh/initHooks.h#L51-L57

So there is progress, though much remains to be done.

this could be an opportunity to make things
a little bit better.

Absolutely.  I think someone (hint hint) should investigate
the feasibility of two related changes to asyn:

1. Adding an epicsAtExit hook / de-init hook to asyn which
permanently disables some/all ports.

That is, doing the equivalent of asynManager::enable(, 0),
while arranging that a subsequent call to enable(, 1)
would not be honored.

I'm not certain whether this epicsAtExit could be installed
unconditionally, or only in the context of an IOC (using
initHookAfterIocRunning)?  And then whether this could be
done for all versions of Base, or only some?

fyi. in Base 7.0.2 the initHooks code was moved to libCom,
which may help with libasyn dependencies.  The de-init hooks
are added in 7.0.4.

https://github.com/epics-base/epics-base/commits/7.0/modules/libcom/src/iocsh/initHooks.h

2. Add a new interface (eg. named "asynPortShutdown") as
a hook for driver specific cleanup actions.

I suspect it would help avoid some ordering problems if this
new epicsAtExit callback first disabled all ports, then looped
again to execute any asynPortShutdown callbacks.

3. See if this could integrate with the asynPortDriver class lifetime.

Adding a new virtual method to asynPortDriver is an easy first step.
A more difficult, but likely "better", solution would be to somehow
start calling ~asynPortDriver.

Of course this will break code written by certain pessimists,
but this is part of my point.  Encouraging lots of driver specific
cleanup now will complicate introducing a uniform solution later.

https://github.com/mdavidsaver/ndwarp/blob/d5447d914060727c38cef93f03d7de6ab085eb3a/warpApp/src/NDPluginWarp.cpp#L74

(to be clear, I'd happily change this code)

4. (maybe) Add a new asynManager call to trigger port shutdown.

eg. to be triggered during a unit test.

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System