Can't destroy and reconstruct asynPortDriver

[mark0n]

When I construct an asynPortDriver with a certain portName, destroy it and then try to re-create it with the same portName (which I think is legitimate) the constructor fails with the following error:

asynPortDriver:asynPortDriver ERROR: Can't register interfaces: Can't register common.

It seems like ~asynPortDriver() is not cleaning up properly.

[mark0n]

Here is some minimal piece of code that reproduces the error:

const char* portName = "aPort";
const int maxAddr = 0;
const int paramTableSize = 1;
const int interfaceMask = asynDrvUserMask|asynInt32Mask;
const int interruptMask = asynInt32Mask;
const int asynFlags = 0;
const int autoConnect = 0;
const int priority = 0;
const int stackSize = epicsThreadGetStackSize(epicsThreadStackSmall);
{
  asynPortDriver aPort = asynPortDriver(portName, maxAddr, paramTableSize, interfaceMask, interruptMask, asynFlags, autoConnect, priority, stackSize);
} // aPort gets destroyed here
{
  asynPortDriver aPort = asynPortDriver(portName, maxAddr, paramTableSize, interfaceMask, interruptMask, asynFlags, autoConnect, priority, stackSize);
}

[MarkRivers]

Hi Martin, I'm on vacation so I can't test this. But the underlying problem is a limitation in asynManager. The first thing that asynPortDriver does (indirectly) is call pasynManager->registerPort("portName"). The problem is that asynManager does not provide a mechanism to "unregister" or delete a port after it has been created. I am surprised that the first error you saw was about not being able to register the asynCommon interface, I would have thought that there would first be an error about trying to register a duplicate port name.

[mark0n]

Please see https://github.com/mark0n/asyn-1/commits/add-asynmanager-unregisterport for a first attempt in fixing this. With these changes my unit tests aren't failing anymore :-) Note however that so far this seems to be only partly successful - I can see two deficiencies:

• I believe that pport->asynManagerLock should be destroyed by unregisterPort() but right now doing so causes problems with a call to freeAsynUser() from ~asynPortDriver(). Maybe @MarkRivers has an idea what might be wrong here. For now I've commented out the corresponding line in unregisterPort().
• Valgrind reports a memory leak which seem to be related to pinterruptBase in registerInterruptSource. Not sure where to free this resource. Maybe we need to implement a unregisterInterruptSource function?

[MarkRivers]

C++ drivers written using asynPortDriver will presumably have a destructor that does the work of tearing down the port driver, and then calls pasynManager->unregisterPort() at the end. Many such drivers do create object worker threads, so those need to cleanly shut down as well.

How will C drivers work? Most C drivers are currently written to only clean up using epicsAtExit(), i.e.they assume that they only need to clean up when the IOC shuts down. One possibility is that we introduce a new asynExceptionShutdown exception, and pasynManager->unregisterPort calls all registered exception handlers for that port with asynExceptionShutdown. C drivers that need to clean up would need to register an exception handler. That would get called when the port is being destroyed and can do the appropriate cleanup. asynManager could register an epicsAtExit function itself which unregisters all ports as part of IOC shutdown. Then C drivers would not have to worry about epicsAtExit, they would only have to write an asyn exception handler.

[MarkRivers]

I think this issue is worth pursuing, but I suspect it will take a while to finish it, so I would like to postpone until after asyn R4-31 is released.

[mark0n]

C has manual resource management: If you allocate some sort of a resource (memory, file handle,...) you have to free/return it once you're done using it. In the same way if you run pasynManager->registerPort() you have to run pasynManager->unregisterPort() at the appropriate time (maybe using epicsAtExit()). Getting this right for all corner cases is a challenge. That's why simple-minded guys like myself are preferring C++ where they can bind the life time of their object to some existing scope and thus can be sure that the destructor gets called no matter what.

[MarkRivers]

I don't really see any point in calling pasynManager->unregisterPort() using an epicsAtExit callback. What is the point, since all of the resources are about to be freed by the OS anyway? It seems to me that these resources only need to be freed if one wants to delete the resources associated with the port while continuing to run the application. And in that case we need a way to notify the asyn port driver that the user would like to clean it up. That's why I suggested a new asynException.

But is this worth the effort, do people really want to delete asyn port drivers while continuing to run the application?

[mark0n]

The main advantage of calling unregisterPort() from epicsAtExit() would be to make the IOC free of memory leaks. That would make it easier to use memory checkers like valgrind.

But the problem at hand is that not destroying the driver object properly causes havoc with our unit tests for our driver code. The reason is that our test framework (Google Test) destroys our driver object at the end of each test and recreates it for the next test (which makes sense as this helps to ensure that tests are independent). We found an ugly way to work around this problem but would rather see this fixed in Asyn. My patch (see above) allows us to run our tests properly. Note however that this still needs some polishing to iron out the memory leaks I mentioned above.

[ulrikpedersen]

But is this worth the effort, do people really want to delete asyn port drivers while continuing to run the application?

I'm in favour of having asyn ports closing down/deleting properly for exactly the reasons Martin mention: unittest frameworks tend to create and destroy objects and valgrind really requires everything to be properly freed/deleted or its output is nearly useless. The possibility of running valgrind to track down memory leaks is really useful, especially in asyn drivers which tends to run "forever" causing small memory leaks to accumulate over time.

[ralphlange]

+1 EPICS Base had been relying on the OS cleaning up for decades, and it meant a lot of effort to add the proper clean-up code in the last 5 years when a large number of unit tests were added.

[ericonr]

#171 has been merged, but I don't think it handles fully de-registering ports.