Destructible ports #171

exzombie · 2023-03-08T18:00:37Z

This PR extends asynManager to allow a port to be destructible, solving #172. It complements epics-base/epics-base#283.

Goals

Provide a function in asynManager and a corresponding iocsh command that shuts down a port.
Shut down ports on IOC exit.
The port cannot be re-enabled.
The low-level driver is given a chance to release resources, or outright destroy itself.
Access to the low-level driver is prevented.
The asynPortDriver base class uses the new functionality.
All of this is opt-in so that existing drivers continue to work as they always did.

Port shutdown

There is a new function, asynManager::shutdown(), which

disables the port, short-circuiting asynManager::queueRequest() and asynManager::lockPort(), thus preventing holders of asynUser dangling references from using them;
nullifies all driver pointers that are reachable from the port, none of which should be reachable from user code thanks to the previous point;
marks the port as defunct, which prevents re-enabling it;
triggers a new exception, asynExceptionShutdown, which is to be handled by a conforming driver.

There is a new iocshell command that shuts down a port, useful mostly for testing.

This functionality is opt-in. A conforming driver declares that it is destructible by passing a new attribute, ASYN_DESTRUCTIBLE. In such a case, asynManager will register an epicsAtExit hook to call shutdown() on IOC exit. A conforming driver has to handle asynExceptionShutdown to perform cleanup, and may invalidate itself completely by calling delete.

asynPortDriver and inheritance

asynPortDriver uses the new functionality by

passing through ASYN_DESTRUCTIBLE if it is given to it via the asynFlags parameter;
adding an exception handler that deletes the driver, relying on the destructor for cleanup;
adding a new virtual function, asynPortDriver::shutdown(), which allows things not possible in a destructor;
calling pasynManager::shutdown() from its destructor (while preventing recursion) so that calling delete on the driver also disables the port.

Because this functionality is opt-in, there's a question of backwards compatibility in an inheritance tree, such as those in areaDetector. This can be solved by prescribing that a driver may only receive the ASYN_DESTRUCTIBLE flag and act upon it, but may not force it. This flag should come in via the iocshell command that instantiates a driver. This way, the driver will be destructible only if the most derived class is instantiated with this flag.

Limitations

It is not possible to release all memory because asyn was not designed in a way to allow that. A port and all the structures describing it will continue to exist indefinitely, as will those asynUser instances that the users don't destroy. But, as stated in asyn ports cannot be destroyed on IOC shutdown #172, memory is not the resource that anyone is concerned about on IOC shutdown.
Deleting a subclass of asynPortDriver directly, without calling asynManager::shutdown(), can work, but only if the most derived class calls asynPortDriver::shutdown() from its destructor. This is encouraged by the documentation but not enforced. It only affects unit tests that want to delete drivers, to my knowledge.

TODO

documentation
tests

AppVeyorBot · 2023-03-09T02:57:31Z

❌ Build asyn 1.0.161 failed (commit a796d33d75 by @exzombie)

exzombie · 2023-03-09T17:47:42Z

I've simplified things a bit since yesterday, and did a smoke test with areaDetector. I noticed that its plugins already have working destructors, so I just forced the NDPosPlugin to declare itself as destructible. valgrind reported no new issues. The base class destructor in asynPortDriver ensures that the port is marked as defunct, so even if a driver is deleted directly, everything should be fine.

@MarkRivers, where do I update the documentation? I noticed that you just added a whole new subdirectory.

AppVeyorBot · 2023-03-09T23:54:09Z

❌ Build asyn 1.0.176 failed (commit f56e71eb33 by @exzombie)

AppVeyorBot · 2023-03-10T04:28:22Z

✅ Build asyn 1.0.177 completed (commit 35add117e6 by @exzombie)

MarkRivers · 2023-03-10T15:01:07Z

where do I update the documentation? I noticed that you just added a whole new subdirectory.

The documentation is now in docs/source/.rst. I have deleted all of the old documentation/.html files that have been converted to .rst. There are still a few small files left to do.

There is a Github Actions that publishes the docs to https://epics-modules.github.io/asyn

AppVeyorBot · 2023-03-10T20:07:46Z

✅ Build asyn 1.0.196 completed (commit 921b11a85d by @exzombie)

exzombie · 2023-03-15T20:28:27Z

I've rebased, wrote the documentation, and extended the example driver. While writing the docs, I noticed that asynManager::lockPort doesn't go through the request queue as I had thought. Now, lockPort also returns an error.

I also reintroduced the asynPortDriver::shutdown() function. I had removed it because it seemed to me that things would be simpler without it, and I didn't see a good use case. However, I have taken a close look at some of the scars I got in the past when dealing with a design that relied on calling overridden virtual functions from a base class destructor. Not a good idea.

MarkRivers

Rather than having every driver create an exiting_ flag and set it in the destructor, would it be nicer to provide an asynManager method pasynManager->isExiting(), and/or an asynPortDriver method isExiting()?

AppVeyorBot · 2023-03-16T07:55:06Z

✅ Build asyn 1.0.214 completed (commit 33edcb38c1 by @exzombie)

AppVeyorBot · 2023-03-16T12:48:41Z

✅ Build asyn 1.0.215 completed (commit b66b9a3621 by @exzombie)

exzombie · 2023-03-17T13:00:02Z

Rather than having every driver create an exiting_ flag and set it in the destructor, would it be nicer to provide an asynManager method pasynManager->isExiting(), and/or an asynPortDriver method isExiting()?

Ah, you're referring to the changes I made to the example driver. Well, having
an exit flag and having threads act on it is a standard pattern, so it might
indeed make sense to provide this flag. Maybe. As I see it, there's two aspects
to consider: how such a mechanism would affect the shutdown, and how to provide
it in the first place.

Let's suppose there exists an isExiting() function, and that all subclasses
use it. Meaning, the threads spawned by each subclass consult this function to
decide when to return. And when I say "subclasses", I mean a linear inheritance
chain starting with asynPortDriver. It is possible for each subclass to create
as many background threads as it wants.

First, setting an "exit" flag may not be enough on its own. Often, the
destructor (or shutdown()) has to wake sleeping threads. This is the case for,
say, the example driver. And then, the destructor needs to wait for the thread
to finish, preferably by joining it. In this context, having isExiting()
provided by asynPortDriver base class instead of the subclass is only a minor
improvement, I think.

Second, the flag used by isExiting() is set once, and seen by all the threads
of all subclasses in parallel. I'm not sure whether this is fine or not. I can
imagine a subclass wanting to do a thing or two in shutdown() while its base
class is operating normally. This is not possible if isExiting() stops all
threads at once.

Which brings us to the question: who sets the exiting flag? I had written a
long-ish explanation of how things could work and why most of it is not a good
idea, but I scrapped the text because, well, most if it is not a good idea. So
let me just say that the only way I see this working well is if shutdown is
only allowed to be triggered by asynManager, and a driver may never be
deleted directly. Which would mean that all the tests need to be fixed. Luckily,
it could be enforced, softly, by printing an error message in asynPortDriver
destructor if it executed while the port was still alive.

Is it worth having all threads halt at the same time, and not being able to directly
delete drivers, just to avoid having one boolean per subclass?

MarkRivers · 2023-03-17T16:34:45Z

For this PR I think it would be good to create an Issue that contains the following:

The "problems" that this PR is intended to address. For example:
- IOCs crash when exiting?
- IOCs hang when exiting?
- IOCs leak resources when exiting?
- Inability to delete a driver and leave the IOC running?
- Others?
The goals of this PR in addressing these problems
Limitations
- For example asynManager cannot delete ports so some resource leak is unavoidable

exzombie · 2023-03-17T16:46:27Z

Sure, I can add some background. But why open an issue? This stuff belongs to the cover letter of the PR, no? Especially the goals and limitations.

prjemian · 2023-03-17T17:01:41Z

General advice on issues vs. PRs:

issues: describe some situation that needs attention, comments supply supporting details
PR: a resolution to the issue and discussion about the implementation

The Why? question is the most expensive question that we encounter. An issue provides a direct explanation why a specific software change was initiated.

prjemian · 2023-03-17T17:04:55Z

Issues describe why, PRs describe how.

exzombie · 2023-03-17T17:16:51Z

In other words, the "problems" go into the issue, and the "goals" and "limitations" go into the description above. Clear enough. The reason I asked is that, personally, I have no problem if the problem itself is described in the PR, with no corresponding issue ticket. And I was just wondering if you want a separate issue for some functional reasons, or if it was just aesthetics. In any case, I'll open the issue with the "problem", no problem :)

MarkRivers · 2023-03-17T17:26:19Z

And I was just wondering if you want a separate issue for some functional reasons, or if it was just aesthetics.

Aesthetics, along the lines of what @prjemian said.

ericonr · 2023-04-13T19:12:32Z

asyn/miscellaneous/asynShellCommands.c

+static const iocshFuncDef asynShutdownPortDef =
+    {"asynShutdownPort", 1, asynShutdownPortArgs
+#ifdef IOCSHFUNCDEF_HAS_USAGE
+     ,
+#else
+    };
+static const char asynShutdownPortUsage[] =
+#endif
+    "Permanently disables the port and destroys the port driver,\n"
+    "releasing its resources.\n"
+#ifdef IOCSHFUNCDEF_HAS_USAGE
+    };
+#define asynShutdownPortUsage asynShutdownPortDef.usage
+#else
+    ;
+#endif


A much cleaner way to optionally define this struct member, without this ifdef forest, is with something like what we have in one of our projects.

It is an eye sore, isn't it? I try to follow the existing style, so I just copied the existing pattern. However, I see that there are only three occurrences of this at the moment, so it's not too much work to use the opportunity and prettify all three. It's not worth the overhead of a separate pull request.

ericonr · 2023-04-13T19:16:23Z

asyn/asynDriver/asynManager.c

+        return asynError;
+    }
+
+    // Disabling the port short-circuits queueRequest(), preventing usage of


The standard for comments seems to be /* ... */

In this file, yes. Will fix.

ericonr · 2023-04-13T19:18:08Z

asyn/asynPortDriver/asynPortDriver.cpp

+    if (cbThread)
+        delete cbThread;


AFAIK it's safe to delete a nullptr, so you shouldn't need the conditional.

I see now this was removed in a following commit.

ericonr · 2023-04-13T19:19:17Z

asyn/asynPortDriver/asynPortDriver.h

    void initialize(const char *portNameIn, int maxAddrIn, int interfaceMask, int interruptMask, int asynFlags,
                    int autoConnect, int priority, int stackSize);
+
+    asynParamSet* paramSet;


It might be nice to document this member with doxygen as well.

Perhaps, but I didn't introduce it; it registers as a change because of incidental reordering with initialize() that happened as the PR evolved.

ericonr · 2023-04-13T19:22:01Z

asyn/asynDriver/asynManager.c

-        epicsSnprintf(pasynUser->errorMessage,pasynUser->errorMessageSize,
-            "asynManager:destroyPortDriver cannot connect to port");
+    if(status != asynSuccess) {
+        printf("%s\n", pasynUser->errorMessage);


I realize we can't use pasynUser->errorMessage for the errors in this function, but it feels wrong to print them to stdout, nonetheless.
It feels more reasonable when done in the iocsh commands, since those are usually interactive.

This function is a consumer of pasynUser->errorMessage. It is run at IOC exit, and if an error happens, it's supposed to present it to the user somehow. The only alternative I see is errlog, but it seems like a deviation given that printf() is used for this purpose elsewhere (e.g. in registerPort(). And given that the process is going away, printing this through the errlog task may not be the best idea. Or is it?

ericonr · 2023-04-13T19:22:34Z

asyn/asynPortDriver/asynPortDriver.cpp

    if (exception == asynExceptionShutdown) {
+        // This code is only excuted once: asynManager will not raise the


Comment style

Unlike asynManager.c which has a consistent comment style, this is not the case for the rest of the asyn code. asynPortDriver.cpp doesn't have many //-style comments, but I don't see a reason not to introduce more.

exzombie · 2023-04-18T11:28:12Z

@ericonr Thank you very much for the review!

AppVeyorBot · 2023-04-18T16:21:10Z

✅ Build asyn 1.0.222 completed (commit 39e3cc2b48 by @exzombie)

MarkRivers · 2023-05-24T17:11:45Z

@anjohnson, @mdavidsaver, and @ralphlange could you please look at this? I would like to merge into the next asyn release if you are good with it. Thanks.

anjohnson

I haven't been through all the code changes in detail, but it looks good to me and is definitely worth doing.

ralphlange · 2023-05-25T16:21:04Z

Looks good and appropriate to me.

AppVeyorBot · 2024-06-12T02:21:26Z

❌ Build asyn 1.0.267 failed (commit 32ca62eacb by @exzombie)

The port driver needs to declare that it supports destruction by passing a new flag, ASYN_DESTRUCTIBLE, when registering the port. It also needs to handle the new asynExceptionShutdown. A new function is added, asynManager::shutdown(). It disables the port, nullifies the private driver pointers reachable from the port, and triggers the shutdown exception.

Shutdown is enabled by passing ASYN_DESTRUCTIBLE through asynFlags of the constructor.

The destructor is only registered if the port is declared destructible.

This affects the iocshell command and the epicsAtExit hook. Error message handling has also been improved.

Derived classes might want to stop things in their destructors, not in shutdown(). So we should only stop cbThread at the very end.

This would *always* be printed because shutdown is now called from the asynPortDriver destructor.

This avoids shadowing a system call by a static function. The corresponding field of `asynManager` was also renamed for consistency.

This fixes a segfault for a number of iocsh commands, thought not all.

Also, add the check for a disabled port to queueLockPort. Previously, this function would only return asynDisabled on a blocking port (courtesy of queueRequest), but didn't check on a synchronous port.

This includes changes to the documentation and making the needsShutdown flag atomic. It is clarified that the shutdown function is called unlocked, and that it has to be called when destroying the driver manually.

When the port driver is destroyed, the drvPvt in the interfaces is set to NULL. A destructible driver (which asynPortDriver can be) must be able to handle this.

The mutex that protects the enabled flag, defunct flag, and the interface list is asynManagerLock, not synchronousLock.

The interfaces are not destroyed together with the port. asynPortManager will overwrite the driver pointer with NULL. This allows the interfaces to gracefully deny access to non-existing driver.

This prevents device support code from accessing the interfaces of a port that was destroyed befort initialization code has run.

This significantly reduces the chance of name collision and unintentional function override for existing drivers.

We have encountered subclassed drivers that directly access the protected `asynStdInterfaces` field. Turning it into a pointer broke things, so, this commit makes it a reference instead.

exzombie · 2024-06-12T16:05:22Z

I encountered drivers that access the asynStdInterfaces member variable directly instead of through getAsynStdInterfaces(), which means that changing it to a pointer breaks things. To maintain API compatibility, I added a reference member variable of the same name; the pointer needed.

I also noticed that a lot of drivers have a shutdown() function. No surprise there, but there's more of them than I expected. I thus renamed the new function in the base class to shutdownPortDriver() to avoid breaking existing drivers.

AppVeyorBot · 2024-06-12T19:15:31Z

❌ Build asyn 1.0.268 failed (commit 4098fe0f52 by @exzombie)

AppVeyorBot · 2024-06-13T01:48:51Z

✅ Build asyn 1.0.269 completed (commit 7015fe467b by @exzombie)

exzombie marked this pull request as ready for review March 10, 2023 08:29

exzombie force-pushed the destructible_ports branch 2 times, most recently from f518e9e to f24071a Compare March 15, 2023 20:13

MarkRivers reviewed Mar 15, 2023

View reviewed changes

ericonr reviewed Apr 13, 2023

View reviewed changes

MarkRivers assigned ralphlange, mdavidsaver and anjohnson May 24, 2023

anjohnson approved these changes May 24, 2023

View reviewed changes

ralphlange self-requested a review May 25, 2023 16:21

exzombie force-pushed the destructible_ports branch from 96e12b0 to eb0e698 Compare June 11, 2024 15:49

exzombie added 23 commits June 12, 2024 09:30

Implement shutdown in asynPortDriver

bcf9779

Shutdown is enabled by passing ASYN_DESTRUCTIBLE through asynFlags of the constructor.

When registering a port, register an epicsAtExit destructor

59974c3

The destructor is only registered if the port is declared destructible.

Lock the port while shutting down.

965185b

This affects the iocshell command and the epicsAtExit hook. Error message handling has also been improved.

Add asynPortDriver shutdown tests

2b4f27a

Stop cbThread in destructor, not shutdown()

43fee6d

Derived classes might want to stop things in their destructors, not in shutdown(). So we should only stop cbThread at the very end.

Disable asyn port on asynPortDriver deletion

d251033

Don't print the "already shut down" message

308d211

This would *always* be printed because shutdown is now called from the asynPortDriver destructor.

Do not allow locking a defunct port

fb9fe11

Extend documentation and example asynPortDriver with ASYN_DESTRUCTIBLE

a21a536

testAsynPortDriver: don't try to join thread when not supported

915d9cc

Prettify the definition if iocsh usage messages

8d650aa

Change comment style in asynManager.c for consistency

538191c

Rename shutdown() to shutdownPort()

3004a12

This avoids shadowing a system call by a static function. The corresponding field of `asynManager` was also renamed for consistency.

Fix connectDevice() to check if portName is NULL

139089b

This fixes a segfault for a number of iocsh commands, thought not all.

Lock the port before checking whether it's defunct

13c0258

Also, add the check for a disabled port to queueLockPort. Previously, this function would only return asynDisabled on a blocking port (courtesy of queueRequest), but didn't check on a synchronous port.

asynPortDriver: make the rules for calling shutdown clearer

f9f70d9

This includes changes to the documentation and making the needsShutdown flag atomic. It is clarified that the shutdown function is called unlocked, and that it has to be called when destroying the driver manually.

Ensure asynPortDriver interfaces handle a NULL drvPvt

b5c03f5

When the port driver is destroyed, the drvPvt in the interfaces is set to NULL. A destructible driver (which asynPortDriver can be) must be able to handle this.

Don't manually lock the port when sutting it down

f3c4218

The mutex that protects the enabled flag, defunct flag, and the interface list is asynManagerLock, not synchronousLock.

Keep asynPortDriver interfaces around after destruction

1a1a3f8

The interfaces are not destroyed together with the port. asynPortManager will overwrite the driver pointer with NULL. This allows the interfaces to gracefully deny access to non-existing driver.

Deny findInterface and interrupt client functions on a destroyed port

d82368e

This prevents device support code from accessing the interfaces of a port that was destroyed befort initialization code has run.

Rename shutdown() to shutdownPortDriver()

71f7b71

This significantly reduces the chance of name collision and unintentional function override for existing drivers.

Add a compatibility alias for asynPortDriver interfaces

d066b68

We have encountered subclassed drivers that directly access the protected `asynStdInterfaces` field. Turning it into a pointer broke things, so, this commit makes it a reference instead.

exzombie force-pushed the destructible_ports branch from eb0e698 to d066b68 Compare June 12, 2024 13:48

Remove use of atomics, not available on EPICS 3.14

30b8c87

		if (exception == asynExceptionShutdown) {
		// This code is only excuted once: asynManager will not raise the

Destructible ports #171

Are you sure you want to change the base?

Destructible ports #171

Conversation

exzombie commented Mar 8, 2023 • edited Loading

Goals

Port shutdown

asynPortDriver and inheritance

Limitations

TODO

AppVeyorBot commented Mar 9, 2023

exzombie commented Mar 9, 2023

AppVeyorBot commented Mar 9, 2023

AppVeyorBot commented Mar 10, 2023

MarkRivers commented Mar 10, 2023

AppVeyorBot commented Mar 10, 2023

exzombie commented Mar 15, 2023

MarkRivers left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Mar 16, 2023

AppVeyorBot commented Mar 16, 2023

exzombie commented Mar 17, 2023

MarkRivers commented Mar 17, 2023 • edited Loading

exzombie commented Mar 17, 2023

prjemian commented Mar 17, 2023

prjemian commented Mar 17, 2023

exzombie commented Mar 17, 2023

MarkRivers commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exzombie commented Apr 18, 2023

AppVeyorBot commented Apr 18, 2023

MarkRivers commented May 24, 2023

anjohnson left a comment

Choose a reason for hiding this comment

ralphlange commented May 25, 2023

AppVeyorBot commented Jun 12, 2024

exzombie commented Jun 12, 2024

AppVeyorBot commented Jun 12, 2024

AppVeyorBot commented Jun 13, 2024

exzombie commented Mar 8, 2023 •

edited

Loading

MarkRivers commented Mar 17, 2023 •

edited

Loading