Remove synchronization from methods in androidx.core
Fixes: 187449306
Test: ContextCompatTest, ContentLoadingProgressBarTest
Change-Id: I027c6b366f81e66cad555eaaecc9d427f63a854d
diff --git a/core/core/lint-baseline.xml b/core/core/lint-baseline.xml
index 63626a6..cbe1a6d 100644
--- a/core/core/lint-baseline.xml
+++ b/core/core/lint-baseline.xml
@@ -46,39 +46,6 @@
</issue>
<issue
- id="BanSynchronizedMethods"
- message="Use of synchronized methods is not recommended"
- errorLine1=" /**"
- errorLine2=" ^">
- <location
- file="src/main/java/androidx/core/widget/ContentLoadingProgressBar.java"
- line="92"
- column="5"/>
- </issue>
-
- <issue
- id="BanSynchronizedMethods"
- message="Use of synchronized methods is not recommended"
- errorLine1=" /**"
- errorLine2=" ^">
- <location
- file="src/main/java/androidx/core/widget/ContentLoadingProgressBar.java"
- line="118"
- column="5"/>
- </issue>
-
- <issue
- id="BanSynchronizedMethods"
- message="Use of synchronized methods is not recommended"
- errorLine1=" private synchronized static File createFilesDir(File file) {"
- errorLine2=" ^">
- <location
- file="src/main/java/androidx/core/content/ContextCompat.java"
- line="602"
- column="5"/>
- </issue>
-
- <issue
id="BanUncheckedReflection"
message="Calling Method.invoke without an SDK check"
errorLine1=" requestRelaunchActivityMethod.invoke(activityThread,"
diff --git a/core/core/src/main/java/androidx/core/content/ContextCompat.java b/core/core/src/main/java/androidx/core/content/ContextCompat.java
index 882f35b..2f54ab4 100644
--- a/core/core/src/main/java/androidx/core/content/ContextCompat.java
+++ b/core/core/src/main/java/androidx/core/content/ContextCompat.java
@@ -158,6 +158,9 @@
private static final Object sLock = new Object();
+ // Lock that provides similar functionality to ContextImpl.mSync.
+ private static final Object sSync = new Object();
+
private static TypedValue sTempValue;
/**
@@ -599,18 +602,23 @@
}
}
- private synchronized static File createFilesDir(File file) {
- if (!file.exists()) {
- if (!file.mkdirs()) {
- if (file.exists()) {
- // spurious failure; probably racing with another process for this app
+ private static File createFilesDir(File file) {
+ // In the platform, all operations on Context that involve creating files (codeCacheDir,
+ // noBackupFilesDir, etc.) are synchronized on a single lock owned by the Context. So, if
+ // we lock on a single static lock owned by ContextCompat then we're a bit too broad but
+ // at least we'll provide similar guarantees.
+ synchronized (sSync) {
+ if (!file.exists()) {
+ if (file.mkdirs()) {
return file;
+ } else {
+ // There used to be another check for file.exists() here, but that was a
+ // side-effect of improper synchronization.
+ Log.w(TAG, "Unable to create files subdir " + file.getPath());
}
- Log.w(TAG, "Unable to create files subdir " + file.getPath());
- return null;
}
+ return file;
}
- return file;
}
/**
diff --git a/core/core/src/main/java/androidx/core/widget/ContentLoadingProgressBar.java b/core/core/src/main/java/androidx/core/widget/ContentLoadingProgressBar.java
index e24b6a1..2eb84ef 100644
--- a/core/core/src/main/java/androidx/core/widget/ContentLoadingProgressBar.java
+++ b/core/core/src/main/java/androidx/core/widget/ContentLoadingProgressBar.java
@@ -23,44 +23,35 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
+import androidx.annotation.UiThread;
/**
* ContentLoadingProgressBar implements a ProgressBar that waits a minimum time to be
* dismissed before showing. Once visible, the progress bar will be visible for
* a minimum amount of time to avoid "flashes" in the UI when an event could take
- * a largely variable time to complete (from none, to a user perceivable amount)
+ * a largely variable time to complete (from none, to a user perceivable amount).
*/
public class ContentLoadingProgressBar extends ProgressBar {
- private static final int MIN_SHOW_TIME = 500; // ms
- private static final int MIN_DELAY = 500; // ms
+ private static final int MIN_SHOW_TIME_MS = 500;
+ private static final int MIN_DELAY_MS = 500;
+ // These fields should only be accessed on the UI thread.
long mStartTime = -1;
-
boolean mPostedHide = false;
-
boolean mPostedShow = false;
-
boolean mDismissed = false;
- private final Runnable mDelayedHide = new Runnable() {
-
- @Override
- public void run() {
- mPostedHide = false;
- mStartTime = -1;
- setVisibility(View.GONE);
- }
+ private final Runnable mDelayedHide = () -> {
+ mPostedHide = false;
+ mStartTime = -1;
+ setVisibility(View.GONE);
};
- private final Runnable mDelayedShow = new Runnable() {
-
- @Override
- public void run() {
- mPostedShow = false;
- if (!mDismissed) {
- mStartTime = System.currentTimeMillis();
- setVisibility(View.VISIBLE);
- }
+ private final Runnable mDelayedShow = () -> {
+ mPostedShow = false;
+ if (!mDismissed) {
+ mStartTime = System.currentTimeMillis();
+ setVisibility(View.VISIBLE);
}
};
@@ -93,13 +84,23 @@
* Hide the progress view if it is visible. The progress view will not be
* hidden until it has been shown for at least a minimum show time. If the
* progress view was not yet visible, cancels showing the progress view.
+ * <p>
+ * This method may be called off the UI thread.
*/
- public synchronized void hide() {
+ public void hide() {
+ // This method used to be synchronized, presumably so that it could be safely called off
+ // the UI thread; however, the referenced fields were still accessed both on and off the
+ // UI thread, e.g. not thread-safe. Now we hand-off everything to the UI thread.
+ post(this::hideOnUiThread);
+ }
+
+ @UiThread
+ private void hideOnUiThread() {
mDismissed = true;
removeCallbacks(mDelayedShow);
mPostedShow = false;
long diff = System.currentTimeMillis() - mStartTime;
- if (diff >= MIN_SHOW_TIME || mStartTime == -1) {
+ if (diff >= MIN_SHOW_TIME_MS || mStartTime == -1) {
// The progress spinner has been shown long enough
// OR was not shown yet. If it wasn't shown yet,
// it will just never be shown.
@@ -109,7 +110,7 @@
// so put a delayed message in to hide it when its been
// shown long enough.
if (!mPostedHide) {
- postDelayed(mDelayedHide, MIN_SHOW_TIME - diff);
+ postDelayed(mDelayedHide, MIN_SHOW_TIME_MS - diff);
mPostedHide = true;
}
}
@@ -118,15 +119,25 @@
/**
* Show the progress view after waiting for a minimum delay. If
* during that time, hide() is called, the view is never made visible.
+ * <p>
+ * This method may be called off the UI thread.
*/
- public synchronized void show() {
+ public void show() {
+ // This method used to be synchronized, presumably so that it could be safely called off
+ // the UI thread; however, the referenced fields were still accessed both on and off the
+ // UI thread, e.g. not thread-safe. Now we hand-off everything to the UI thread.
+ post(this::showOnUiThread);
+ }
+
+ @UiThread
+ private void showOnUiThread() {
// Reset the start time.
mStartTime = -1;
mDismissed = false;
removeCallbacks(mDelayedHide);
mPostedHide = false;
if (!mPostedShow) {
- postDelayed(mDelayedShow, MIN_DELAY);
+ postDelayed(mDelayedShow, MIN_DELAY_MS);
mPostedShow = true;
}
}
