fix: update moduleMap in UnloadPluginLibrary

[tlambert03]

unloadLibrary() unloaded the adapter module but left its entry in CPluginManager::moduleMap_, so a later load under the same name failed ("already loaded") for mock adapters, or reused the stale entry (with function pointers into the unloaded library) for regular adapters; this erases the entry after a successful unload and adds a regression test.

edit: discovered while working on pymmcore-plus/pymmcore-nano#86, where creating and discards adapters at runtime becomes something that is actually more likely/useful.

[marktsuchida]

Good fix, but.... I actually consider that function to be a bit of a mistake for actual (dlopened) adapters. It could work in theory, but in practice only if the adapter and all of its dependencies are carefully crafted to cleanly unload (which is not trivial to ensure in C++). Otherwise the symptom ranges from leaks to inability to load again to crashes (during unload or reload). And our adapters are not crafted in such a way. I would not expect most third-party driver DLLs to be safe to unload either, unless documented to be so (which is rare).

I think unloadLibrary was introduced for quick-and-dirty testing of (particular) device adapters under development but it's a bit too dirty for my taste (and certainly for production use). Hence the doc comment "Experimental. Don't use." (since 2013). One of the issues, even when reloading appears to work on the surface, is that transitive dependencies are only unloaded if their refcount reaches 0, so it's not a clean way to test a device adapter (previous driver state may be carried over).

---

The bug fixed here looks like the result of "fixes" a less experienced version of I made in 2013.

Given that unloading of real adapters is broken, which means hopefully nobody is depending on it currently, I can think of a few ways forward:

1. Expose unloading of real adapters, even though it is only rarely usable in practice and fails poorly in many cases
2. Make unloading of real adatpers always be an error (before even trying)
3. Deprecate/remove CMMCore::unloadLibrary()

In all 3 cases, mock adapters (which of course we'll rename for Python) can still be unloaded (using a new function in case 3).

I think I favor 2. What do you think?

Of course, in the future we could support unloading of real adapters that explicitly declare their support for doing so, which could be useful for adapters that don't have third-party dependencies (static or dynamic).

---

We kind of need good terms for dlopened vs "mock" adapters. I don't like adjectives like "library", "dynamic", "static", "external", "built-in", which are either inaccurate or ambiguous. One LLM suggestion is "loaded" vs "registered" device adapters. If we choose these, we might want a hybrid between 2 and 3 where unloadLibrary() always throws an error while we introduce unregisterDeviceAdapter() for the registered ones. But if we have other ideas for the terminology, sharing unloadLibrary() might be fine. I do kind of like keeping the load/unload verbs for all adapters, given how entrenched "load" is in Micro-Manager.

[tlambert03]

makes sense!

yeah, I'm good with option 2 (assuming you mean it would be an error when unloadLibrary is called on a real adaptor, but succeeds with a registered device). I'm also ok with removing CMMCore::unloadLibrary() and adding a new one (slightly prefer 2 only because it avoids a new public method).

closing this, since either way this isn't the right approach

[tlambert03]

actually, reopening with one possible way to implement option 2. let me know if this is what you meant

One caveat: CMMCore::unloadLibrary() unloads the adapter's devices before it reaches Unload(), so for a DLL adapter the devices are unloaded and then the call throws. The adapter itself stays loaded and usable. If you'd rather reject before touching any devices, I have a version that checks up front (a SupportsUnload() virtual on the adapter impl, checked first thing in unloadLibrary()). Happy to switch to it.

[marktsuchida]

Yes, that is what I had in mind! Your caveat raises a good point, but I think it's okay -- it's more common that we cannot predict errors before taking action, so no need to overachieve here.

We're going from "experimental, don't use" + unpredictable behavior to explicit error for classic adapters, so I don't think we really need to worry about breaking anybody's code that wasn't already broken. But I will increment the MMCore patch version before merging.

[marktsuchida]

Would it be useful to have a method to query the type (classic vs registered) of an adapter? (So the app can determine if it is unloadable.)

[tlambert03]

Would it be useful to have a method to query the type (classic vs registered) of an adapter? (So the app can determine if it is unloadable.)

I don't have an immediate need for it, but also don't have any objections

[marktsuchida]

Well, let's not bother until it's needed then!