-
Notifications
You must be signed in to change notification settings - Fork 6.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tidy up and share (between processes) temporary copies of JNI shared library loaded from JAR #13318
Draft
alanpaxton
wants to merge
24
commits into
facebook:main
Choose a base branch
from
evolvedbinary:eb/share-tmp-jar-lib-files
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Tidy up and share (between processes) temporary copies of JNI shared library loaded from JAR #13318
alanpaxton
wants to merge
24
commits into
facebook:main
from
evolvedbinary:eb/share-tmp-jar-lib-files
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The rocksdbjni jar unpacks its library file to temporay space if it is not available on the library load path. In pathological cases, large numbers of these files build up; In particular when processes are killed, rather than being allowed to deleteOnExit the libraries. Our solution is a mechanism to let a single instance of the file be shared by all the processes that want to use it. It’s not perfect, but the worst case should only result in a small fixed number of instances of the file being left in temporary space.
Method `searchOrCreate()` makes standard idiom use of `SharedTempFile` easier Cleanup after themselves (as much as possible) for the multiprocess tests
Renamed test to something more appropriate (SharedTempFileLoaderTest) Added to the list of tests in both make and cmake builds
Unlocking the shared temp file is often run as a cleanup operation when a JVM terminates. It’s possible for instance that other cleanup operations may have removed the directory containing the shared temp file (e.g. junit cleanup). So check for the locking directory’s existence, and quietly do nothing if it is not there.
Put our temporaries inside JUnit temporary when we can Rationalise passing of parameters to test processes (add ArgUtil)
Create our SharedTempFile directory filenames with a digest of a supplied string as a suffix. This will typically be the full name of the library, including version. So when the version changes, the digest will change and the correct library will be picked up, creating a new SharedTempFile directory on disk..
Add a variant shared library loader test that verifies the shared library can be loaded from java.library.path if it exists there; supported by adding a classpath parameter to the process creation code, so we can run a variant of our tests which is unable to find the JAR, but succeeds anyway because it does find the shared library directly.
Flying blind
Missed: src/test/java/org/rocksdb/SharedTempFileMockMain.java src/test/java/org/rocksdb/SharedTempFileRocksDBMain.java when building the test lib via cmake, so no-worky; add them
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #13185
NativeLibraryLoader.java
(part of RocksJava) goes through a number of stages to find an instance of the rocksdb native library to run against. As a backstop, it extracts a copy of the library from the rocksdb jni jar (the build bundles the library into the jar). The library is stored in the temporary directory, and the Java process is requested todeleteOnExit()
the temporary file. This deletion doesn't work when the Java process is killed (-9) and as a result, many copies of the (large) library file can build up.This PR adds a mechanism to find/create a temporary library in a known subdirectory of the temporary directory; the name is based on the name requested by the client, plus some digestification to distinguish between files with version names. The Java file locking mechanism is used to lock a blessed lock file in the subdirectory, and a separate 0-length lock file is created for each process which is using the library. The last process to stop using the library deletes the temporary library, but NOT the directory and the blessed lock file.
In this model, the result of not running shutdown handlers will be to leave 1 instance of each version of the library in place; however, it will be discovered by future processes so that the system will not collect multiple unpacked copies of the same version.