Project

General

Profile

Feature #11678

Allow configuration of cet::LibraryManager search path

Added by Ben Morgan almost 5 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Start date:
02/09/2016
Due date:
% Done:

100%

Estimated time:
16.00 h
Spent time:
Duration:

Description

Currently cet::LibraryManager hardcodes the search path of libraries to the (DY)LD_LIBRARY_PATH in its constructor (source:cetlib/LibraryManager.cc#L59). On OS X platforms this causes issues on El Capitan and above due to the changes in System Integrity Protection - the variable is not available in a subprocess, and thus cet::LibraryManager always throws an exception (thrown from the cet::search_path constructor. This hardcoding also makes it difficult to use cet::LibraryManager by upstream clients as

  • It forces clients to configure loading policy based on a widely used environment variable that can be very fragile
    • Use Case : an application supplies its own set of plugins and can self-locate these via APIs such as binreloc. Clients of the application should not be forced to set the dynamic loader path in this case.
  • No interface is available to prepend/append additional paths for libraries to allow additions
    • Use Case : an application wishes to implement a layered configuration for plugin loading based on builtins -> user config file -> application specific env var.
    • Use Case : an application supplies a GUI through which additional plugin paths can be added

I've started to take a look at both issues, but am not sure of the best solution in order not to break the current cetlib::LibraryManager API and expected behaviour

  • I think there should be a constructor that takes the lib type/suffix, name pattern and the search path (as a cet::search_path instance probably).
  • The current two argument constructor could delegate to this with a cet::search_path instance for (DY)LD_LIBRARY_PATH?
  • Add new methods "prependToSearchPath(...)" and "appendToSearchPath(...)" - ellipsis used as argument could be string or a search_path...
    • This is the major thing I'm not sure about as the current hardcoded search path is processed once and once only in the constructor, plus the various lookup tables are implemented as maps rather than ordered collections.

I realise this is therefore a reasonably complicated feature request, so clearly needs further review - though I'm happy to implement, test and supply patches for any suggestions.

History

#1 Updated by Kyle Knoepfel almost 5 years ago

  • Subject changed from Allow configuration of @cet::LibraryManager@ search path to Allow configuration of cet::LibraryManager search path
  • Status changed from New to Accepted
  • Estimated time set to 4.00 h

Adding a c'tor argument to specify the search path certainly seems reasonable. The time estimate does not include modifying art to move away from using (DY)LD_LIBRARY_PATH; for that issue, we will raise the discussion at the stakeholder meeting.

#2 Updated by Kyle Knoepfel over 4 years ago

  • Target version set to 2.01.00

#3 Updated by Christopher Green over 4 years ago

  • Status changed from Accepted to Assigned
  • Assignee set to Christopher Green
  • Target version changed from 2.01.00 to 2.00.01

#4 Updated by Christopher Green over 4 years ago

  • Status changed from Assigned to Feedback

I was all set to implement the constructor and talk about options for the path prepend / append feature, but I went ahead and checked the OS X man page for dlopen(3) to ensure it had a non-POSIX feature available on Linux that I wished to use (RTLD_NOLOAD). Unfortunately, while OS X's dlopen() does have RTLD_NOLOAD, I found the following:

When path contains a slash but is not a framework path (i.e. a full path or a partial path to a dylib), dlopen() searches the following until it finds a compatible Mach-O file: $DYLD_LIBRARY_PATH (with leaf name from path), then the supplied path (using current working directory for relative paths), then $DYLD_FALLBACK_LIBRARY_PATH (with leaf name from path).

This would seem to indicate that there is no way to do what you want unless we temporarily prepend (at least) the path to your chosen library to DYLD_LIBRARY_PATH right before the dlopen() call. This should work (to within the question of how dependencies should be resolved), but still feels kludgy to me.

Regarding prepending or adding to the search path: the problem is that there is no safe facility for unloading an already-loaded plugin. If LibraryManager has found and loaded a library, chances are the plugin has been utilized and there are extant instances of objects using it. If a modified search path would cause a different plugin to be loaded than the one that is already loaded, there is nothing we can do and now we have an inconsistency issue.

My current thinking is that the only way we could implement a path modification function is to throw an exception if the location for an already-loaded entry would change and a corresponding library has already been loaded. In order to avoid the LibraryManager being in such an inconsistent state that immediate program exit is the only safe course, we would have to have a transaction-like operation, where the results based on the new search path would only replace the current set if there were no problems. Otherwise, we throw an exception because we are unable to do what the user requested in a way that would require code changes to fix.

To expand upon, "has already been loaded" above, we can check whether:

  1. The library corresponding to the existing calculated answer has been loaded;
  2. The library corresponding to that which the system would find according to its rules has been loaded.

We cannot check if a library with the same or different name satisfying the same symbols has already been loaded.

Please let us know your thoughts on these two issues (interaction between specified search path and dlopen() search behavior, and safety issues in the face of prepend / append).

#5 Updated by Ben Morgan over 4 years ago

Christopher Green wrote:

I was all set to implement the constructor and talk about options for the path prepend / append feature, but I went ahead and checked the OS X man page for dlopen(3) to ensure it had a non-POSIX feature available on Linux that I wished to use (RTLD_NOLOAD). Unfortunately, while OS X's dlopen() does have RTLD_NOLOAD, I found the following:

When path contains a slash but is not a framework path (i.e. a full path or a partial path to a dylib), dlopen() searches the following until it finds a compatible Mach-O file: $DYLD_LIBRARY_PATH (with leaf name from path), then the supplied path (using current working directory for relative paths), then $DYLD_FALLBACK_LIBRARY_PATH (with leaf name from path).

This would seem to indicate that there is no way to do what you want unless we temporarily prepend (at least) the path to your chosen library to DYLD_LIBRARY_PATH right before the dlopen() call. This should work (to within the question of how dependencies should be resolved), but still feels kludgy to me.

I don't see (or misunderstand) the issue here. If I pass a path to a library to be loaded, then LibraryManager should

1. Check if this an absolute path, if not,
2. Try and find it under the internal search path, resolving to an absolute path
3. Pass the fully resolved path to dlopen or fail if no absolute path could be derived.

The gotcha that DYLD_LIBRARY_PATH acts as an override even when an absolute path to an existing library is supplied might be exactly what is wanted (that's the exact purpose of the env var). In either case, could a policy/predicate on checking for DYLD_LIBRARY_PATH "shadowing" a library be used? That could check for shadowing (between 2 and 3 above) and fail or warn as determined by the policy implementation.

Regarding prepending or adding to the search path: the problem is that there is no safe facility for unloading an already-loaded plugin. If LibraryManager has found and loaded a library, chances are the plugin has been utilized and there are extant instances of objects using it. If a modified search path would cause a different plugin to be loaded than the one that is already loaded, there is nothing we can do and now we have an inconsistency issue.

My current thinking is that the only way we could implement a path modification function is to throw an exception if the location for an already-loaded entry would change and a corresponding library has already been loaded. In order to avoid the LibraryManager being in such an inconsistent state that immediate program exit is the only safe course, we would have to have a transaction-like operation, where the results based on the new search path would only replace the current set if there were no problems. Otherwise, we throw an exception because we are unable to do what the user requested in a way that would require code changes to fix.

To expand upon, "has already been loaded" above, we can check whether:

  1. The library corresponding to the existing calculated answer has been loaded;
  2. The library corresponding to that which the system would find according to its rules has been loaded.

We cannot check if a library with the same or different name satisfying the same symbols has already been loaded.

Please let us know your thoughts on these two issues (interaction between specified search path and dlopen() search behavior, and safety issues in the face of prepend / append).

I'll have to look at the details/interactions of LibraryManager and PluginFactory to give a more comprehensive answer. Right now, I could live without prepend/append and have them as future "nice to have" features. The more important thing is to have a library search path that's not locked to the environment, even if it can only be set once using a prior assembled list when constructing LibraryManager. I see:

auto basePluginPath = getApplicationPluginPath();
basePluginPath.prepend(getUserConfigPluginPath())
basePluginPath.prepend(otherPath);
// and so on
LibraryManager myManager {basePluginPath};

as being fine to satisfy the majority of use cases.

#6 Updated by Kyle Knoepfel over 4 years ago

  • Target version deleted (2.00.01)

#7 Updated by Ben Morgan over 4 years ago

Just to quickly bump - do you need any further feedback on this issue?

#8 Updated by Kyle Knoepfel over 4 years ago

  • Status changed from Feedback to Accepted

We can make this change, however, we do not believe it will fully address the issue, due to the El Capitan SIP behavior.

#9 Updated by Kyle Knoepfel almost 4 years ago

  • Status changed from Accepted to Feedback

Due to the urgency of other issues, we are not able to take on this issue right now. However, if you are willing to supply patches regarding this issue, we will be happy to review them and apply as appropriate.

#10 Updated by Ben Morgan almost 4 years ago

Attached is a first attempt at providing the core interfaces needed to allow LibraryManager and clients in cetlib (PluginFactory and BasicPluginFactory) to be constructed using a cetlib::search_path argument to the constructors. I have followed as far as possible format/coding style in the editing files, but there is likely some notification to be done, so let me know and I can fix and submit a new patch.

Basic tests have been added to exercise the use of a custom search path by reusing the existing tests with a different test fixture selected at compile time. This was done to ensure identical functionality is tested. All(*) tests pass on macOS Sierra with non-UPS (see https://github.com/drbenmorgan/fnal-cetlib/tree/feature/issue-11678), and also SLC6 using the art-v2_06_00 bundle and buildtool/UPS.

(*) The regex_t test has not been changed, so this will still fail on macOS as it uses the default system search path.

Other than the style issues mentioned above, I think there are a couple of todos remaining:

  • LibraryManager::getSymbolByLibspec_ still outputs an error using the result of os_libpath, which is incorrect when a custom search path is used.
  • Could be addressed by storing the search path as a const cet::search_path member and reporting that?
  • Could cet::search_path record the source its search path, e.g. "string" vs "environment" so extra information on how the path was constructed is available?

The first is the main issue I think.

I also haven't looked at using the new interfaces in downstream clients like MessageFacility, but the patch retains all existing constructors, so should not affect API nor ABI compatibility.

#11 Updated by Kyle Knoepfel almost 4 years ago

  • Status changed from Feedback to Assigned

Thank you, Ben. We will review the patch and apply as appropriate.

#12 Updated by Kyle Knoepfel over 3 years ago

  • Target version set to 1209
  • Estimated time changed from 4.00 h to 16.00 h

#13 Updated by Kyle Knoepfel over 3 years ago

  • Assignee changed from Christopher Green to Kyle Knoepfel

#14 Updated by Kyle Knoepfel over 3 years ago

  • Status changed from Assigned to Resolved
  • % Done changed from 0 to 100

Implemented with commit cetlib:803ab21. The patch applied cleanly. I did make a few adjustments (not only to the patch, but also the extant code):

  • The PluginFactory class still has only one c'tor, but it receives an additional cet::search_path argument. I retained the extra c'tors for the other classes, but I thought it was safe to restrict the API for PluginFactory.
  • Adding a cet::search_path member to LibraryManager was a reasonable solution, as well as providing the context in which that cet::search_path was created. The cet::search_path class now has a showenv() function that returns the environment variable name used to create the search-path object. If an environment variable was not used, then showenv() returns an empty string.

#15 Updated by Ben Morgan over 3 years ago

Kyle Knoepfel wrote:

Implemented with commit cetlib:803ab21. The patch applied cleanly. I did make a few adjustments (not only to the patch, but also the extant code):

  • The PluginFactory class still has only one c'tor, but it receives an additional cet::search_path argument. I retained the extra c'tors for the other classes, but I thought it was safe to restrict the API for PluginFactory.
  • Adding a cet::search_path member to LibraryManager was a reasonable solution, as well as providing the context in which that cet::search_path was created. The cet::search_path class now has a showenv() function that returns the environment variable name used to create the search-path object. If an environment variable was not used, then showenv() returns an empty string.

Thanks Kyle! I've tested locally with cetlib:803ab21 and all works fine for me.

Just a couple of quick questions - the PluginFactory_t and LibraryManager_t tests still fail, so would it make sense to set the test properties on these to "known and expected to fail when running on macOS"? That would just make the test/ctest output cleaner. Also, the regex_t test uses the default search path and thus fails - could this be updated to use the custom search path, or should a separate test like Plugin/LibraryManager_t be added?

In both cases, I'm happy to submit patches in separate Issues if these are felt to be sensible/required.

#16 Updated by Kyle Knoepfel over 3 years ago

  • Target version changed from 1209 to 2.07.01

#17 Updated by Kyle Knoepfel over 3 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF