Skip to content

Commit

Permalink
Simplify retry logic in loadLibraryBySoName
Browse files Browse the repository at this point in the history 
Summary:
There're two retry points in Soloader now, they look redundant. Try to simplify them on this diff. We also did some refactor in this diff:
* Deprecate `LOAD_FLAG_ALLOW_SOURCE_CHANGE`. After we move retry logic to the `loadLibraryBySoName`, we don't need this flag to indicate whether soloader can modify the so source.
* Move test only methods to inner `TestOnlyUtils` class to prevent misusing.Some test functions are not thread-safe.

Reviewed By: MatthewLangille

Differential Revision: D20449988

fbshipit-source-id: c9b8976da84b42d5b7bc0a5bd524fe3ffb5fef8b
  • Loading branch information
@simpleton @facebook-github-bot
simpleton authored and facebook-github-bot committed Mar 16, 2020
1 parent c0d3c13 commit f8eaf3e
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 115 deletions.
13 changes: 5 additions & 8 deletions java/com/facebook/soloader/DirectorySoSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,20 +109,17 @@ public String[] getLibraryDependencies(String soName) throws IOException {
}
}

private void loadDependencies(File soFile, int loadFlags, StrictMode.ThreadPolicy threadPolicy)
throws IOException {
String dependencies[] = getDependencies(soFile);
private static void loadDependencies(
File soFile, int loadFlags, StrictMode.ThreadPolicy threadPolicy) throws IOException {
String[] dependencies = getDependencies(soFile);
Log.d(SoLoader.TAG, "Loading lib dependencies: " + Arrays.toString(dependencies));
for (int i = 0; i < dependencies.length; ++i) {
String dependency = dependencies[i];
for (String dependency : dependencies) {
if (dependency.startsWith("/")) {
continue;
}

SoLoader.loadLibraryBySoName(
dependency,
((loadFlags | LOAD_FLAG_ALLOW_IMPLICIT_PROVISION) & ~LOAD_FLAG_ALLOW_SOURCE_CHANGE),
threadPolicy);
dependency, loadFlags | LOAD_FLAG_ALLOW_IMPLICIT_PROVISION, threadPolicy);
}
}

Expand Down
197 changes: 92 additions & 105 deletions java/com/facebook/soloader/SoLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import java.util.concurrent.locks.ReentrantReadWriteLock;
import javax.annotation.Nullable;
import javax.annotation.concurrent.GuardedBy;
import javax.annotation.concurrent.NotThreadSafe;
import javax.annotation.concurrent.ThreadSafe;

/**
Expand Down Expand Up @@ -452,38 +453,47 @@ private static boolean checkIfSystemApp(Context context) {

/** Turn shared-library loading into a no-op. Useful in special circumstances. */
public static void setInTestMode() {
setSoSources(new SoSource[] {new NoopSoSource()});
TestOnlyUtils.setSoSources(new SoSource[] {new NoopSoSource()});
}

/** Make shared-library loading delegate to the system. Useful for tests. */
public static void deinitForTest() {
setSoSources(null);
TestOnlyUtils.setSoSources(null);
}

/** Set so sources. Useful for tests. */
/* package */ static void setSoSources(SoSource[] sources) {
sSoSourcesLock.writeLock().lock();
try {
sSoSources = sources;
sSoSourcesVersion++;
} finally {
sSoSourcesLock.writeLock().unlock();
@NotThreadSafe
static class TestOnlyUtils {
/* Set so sources. Useful for tests. */
/* package */ static void setSoSources(SoSource[] sources) {
sSoSourcesLock.writeLock().lock();
try {
sSoSources = sources;
sSoSourcesVersion++;
} finally {
sSoSourcesLock.writeLock().unlock();
}
}
}

/** Set so file loader. Only for tests. */
/* package */ static void setSoFileLoader(SoFileLoader loader) {
sSoFileLoader = loader;
}
/**
* Set so file loader. <br>
* N.B. <b>ONLY FOR TESTS</b>. It has read/write race with {@code
* SoLoader.sSoFileLoader.load(String, int)} in {@link DirectorySoSource#loadLibraryFrom}
*
* @param loader {@link SoFileLoader}
*/
/* package */ static void setSoFileLoader(SoFileLoader loader) {
sSoFileLoader = loader;
}

/** Reset internal status. Only for tests. */
/* package */ static void resetStatus() {
synchronized (SoLoader.class) {
sLoadedLibraries.clear();
sLoadingLibraries.clear();
sSoFileLoader = null;
/** Reset internal status. Only for tests. */
/* package */ static void resetStatus() {
synchronized (SoLoader.class) {
sLoadedLibraries.clear();
sLoadingLibraries.clear();
sSoFileLoader = null;
}
setSoSources(null);
}
setSoSources(null);
}

/**
Expand Down Expand Up @@ -610,16 +620,12 @@ public static boolean loadLibrary(String shortName, int loadFlags) throws Unsati
String soName = mergedLibName != null ? mergedLibName : shortName;

return loadLibraryBySoName(
System.mapLibraryName(soName),
shortName,
mergedLibName,
loadFlags | SoSource.LOAD_FLAG_ALLOW_SOURCE_CHANGE,
null);
System.mapLibraryName(soName), shortName, mergedLibName, loadFlags, null);
}

/* package */ static void loadLibraryBySoName(
String soName, int loadFlags, StrictMode.ThreadPolicy oldPolicy) {
loadLibraryBySoName(soName, null, null, loadFlags, oldPolicy);
loadLibraryBySoNameImpl(soName, null, null, loadFlags, oldPolicy);
}

private static boolean loadLibraryBySoName(
Expand All @@ -628,6 +634,44 @@ private static boolean loadLibraryBySoName(
@Nullable String mergedLibName,
int loadFlags,
@Nullable StrictMode.ThreadPolicy oldPolicy) {
boolean ret = false;
boolean retry;
do {
retry = false;
try {
ret = loadLibraryBySoNameImpl(soName, shortName, mergedLibName, loadFlags, oldPolicy);
} catch (UnsatisfiedLinkError e) {
final int currentVersion = sSoSourcesVersion;
sSoSourcesLock.writeLock().lock();
try {
if (sApplicationSoSource != null && sApplicationSoSource.checkAndMaybeUpdate()) {
Log.w(
TAG,
"sApplicationSoSource updated during load: " + soName + ", attempting load again.");
sSoSourcesVersion++;
retry = true;
}
} catch (IOException ex) {
throw new RuntimeException(ex);
} finally {
sSoSourcesLock.writeLock().unlock();
}

if (sSoSourcesVersion == currentVersion) {
// nothing changed in soSource, Propagate original error
throw e;
}
}
} while (retry);
return ret;
}

private static boolean loadLibraryBySoNameImpl(
String soName,
@Nullable String shortName,
@Nullable String mergedLibName,
int loadFlags,
@Nullable StrictMode.ThreadPolicy oldPolicy) {
// While we trust the JNI merging code (`invokeJniOnload(..)` below) to prevent us from invoking
// JNI_OnLoad more than once and acknowledge it's more memory-efficient than tracking in Java,
// by tracking loaded and merged libs in Java we can avoid undue synchronization and blocking
Expand Down Expand Up @@ -672,8 +716,6 @@ private static boolean loadLibraryBySoName(
try {
Log.d(TAG, "About to load: " + soName);
doLoadLibraryBySoName(soName, loadFlags, oldPolicy);
} catch (IOException ex) {
throw new RuntimeException(ex);
} catch (UnsatisfiedLinkError ex) {
String message = ex.getMessage();
if (message != null && message.contains("unexpected e_machine:")) {
Expand Down Expand Up @@ -754,41 +796,8 @@ public static File unpackLibraryAndDependencies(String shortName) throws Unsatis
}

private static void doLoadLibraryBySoName(
String soName, int loadFlags, StrictMode.ThreadPolicy oldPolicy) throws IOException {
// Load the library, and if fails - attempt a single retry to update ApplicationSoSource
try {
doLoadLibraryBySoNameImpl(soName, loadFlags, oldPolicy);
} catch (UnsatisfiedLinkError e) {
if (sApplicationSoSource != null) {
Log.e(TAG, "Failed to load library, attempting reload of ApplicationSoSource.");

boolean updated = false;
sSoSourcesLock.writeLock().lock();
try {
if (sApplicationSoSource.checkAndMaybeUpdate()) {
sSoSourcesVersion++;
updated = true;
}
} finally {
sSoSourcesLock.writeLock().unlock();
}
// If updated - attempt second reload
if (updated) {
Log.e(TAG, "ApplicationSoSource updated, attempting load again.");
doLoadLibraryBySoNameImpl(soName, loadFlags, oldPolicy);
} else {
// Propagate original error
throw e;
}
} else {
// Propagate original error
throw e;
}
}
}

private static void doLoadLibraryBySoNameImpl(
String soName, int loadFlags, StrictMode.ThreadPolicy oldPolicy) throws IOException {
String soName, int loadFlags, @Nullable StrictMode.ThreadPolicy oldPolicy)
throws UnsatisfiedLinkError {

int result = SoSource.LOAD_RESULT_NOT_FOUND;
sSoSourcesLock.readLock().lock();
Expand All @@ -815,50 +824,28 @@ private static void doLoadLibraryBySoNameImpl(

Throwable error = null;
try {
boolean retry;
do {
retry = false;
sSoSourcesLock.readLock().lock();
final int currentSoSourcesVersion = sSoSourcesVersion;
try {
for (int i = 0; result == SoSource.LOAD_RESULT_NOT_FOUND && i < sSoSources.length; ++i) {
SoSource currentSource = sSoSources[i];
result = currentSource.loadLibrary(soName, loadFlags, oldPolicy);
if (result == SoSource.LOAD_RESULT_CORRUPTED_LIB_FILE && sBackupSoSources != null) {
// Let's try from the backup source
Log.d(TAG, "Trying backup SoSource for " + soName);
for (UnpackingSoSource backupSoSource : sBackupSoSources) {
backupSoSource.prepare(soName);
int resultFromBackup = backupSoSource.loadLibrary(soName, loadFlags, oldPolicy);
if (resultFromBackup == SoSource.LOAD_RESULT_LOADED) {
result = resultFromBackup;
break;
}
sSoSourcesLock.readLock().lock();
try {
for (int i = 0; result == SoSource.LOAD_RESULT_NOT_FOUND && i < sSoSources.length; ++i) {
SoSource currentSource = sSoSources[i];
result = currentSource.loadLibrary(soName, loadFlags, oldPolicy);
if (result == SoSource.LOAD_RESULT_CORRUPTED_LIB_FILE && sBackupSoSources != null) {
// Let's try from the backup source
Log.d(TAG, "Trying backup SoSource for " + soName);
for (UnpackingSoSource backupSoSource : sBackupSoSources) {
backupSoSource.prepare(soName);
int resultFromBackup = backupSoSource.loadLibrary(soName, loadFlags, oldPolicy);
if (resultFromBackup == SoSource.LOAD_RESULT_LOADED) {
result = resultFromBackup;
break;
}
break;
}
break;
}
} finally {
sSoSourcesLock.readLock().unlock();
}
if ((loadFlags & SoSource.LOAD_FLAG_ALLOW_SOURCE_CHANGE)
== SoSource.LOAD_FLAG_ALLOW_SOURCE_CHANGE
&& result == SoSource.LOAD_RESULT_NOT_FOUND) {
sSoSourcesLock.writeLock().lock();
try {
// TODO(T26270128): check and update sBackupSoSource as well
if (sApplicationSoSource != null && sApplicationSoSource.checkAndMaybeUpdate()) {
sSoSourcesVersion++;
}
if (sSoSourcesVersion != currentSoSourcesVersion) {
// our list was updated, let's retry
retry = true;
}
} finally {
sSoSourcesLock.writeLock().unlock();
}
}
} while (retry);
} finally {
sSoSourcesLock.readLock().unlock();
}
} catch (Throwable t) {
error = t;
} finally {
Expand Down
4 changes: 2 additions & 2 deletions java/com/facebook/soloader/SoSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,13 @@ public abstract class SoSource {
public static final int LOAD_FLAG_ALLOW_IMPLICIT_PROVISION = 1;

/** Allow loadLibrary to reparse the so sources directories. */
public static final int LOAD_FLAG_ALLOW_SOURCE_CHANGE = 1 << 1;
@Deprecated public static final int LOAD_FLAG_ALLOW_SOURCE_CHANGE = 1 << 1;

/**
* Min flag that can be used in customized {@link SoFileLoader#load(String, int)} implementation.
* The custom flag value has to be greater than this.
*/
public static final int LOAD_FLAG_MIN_CUSTOM_FLAG = LOAD_FLAG_ALLOW_SOURCE_CHANGE << 1;
public static final int LOAD_FLAG_MIN_CUSTOM_FLAG = 1 << 2;

/** Allow prepare to spawn threads to do background work. */
public static final int PREPARE_FLAG_ALLOW_ASYNC_INIT = (1 << 0);
Expand Down

0 comments on commit f8eaf3e

Please sign in to comment.