Skip to content

Commit e3ee00d

Browse files
committed
Fix the logic that decides whether to use currentRequestThreadFactory() under App Engine.
While there, also support thread renaming under App Engine. Fixes #3598 Relevant to #3606 (also, the CL in which I experimented with #3569 before backing it out) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=272446666
1 parent 67dd062 commit e3ee00d

File tree

5 files changed

+31
-38
lines changed

5 files changed

+31
-38
lines changed

android/guava/src/com/google/common/util/concurrent/Callables.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,10 @@ public void run() {
129129
/** Tries to set name of the given {@link Thread}, returns true if successful. */
130130
@GwtIncompatible // threads
131131
private static boolean trySetName(final String threadName, Thread currentThread) {
132-
// In AppEngine, this will always fail. Should we test for that explicitly using
133-
// MoreExecutors.isAppEngine? More generally, is there a way to see if we have the modifyThread
134-
// permission without catching an exception?
132+
/*
133+
* setName should usually succeed, but the security manager can prohibit it. Is there a way to
134+
* see if we have the modifyThread permission without catching an exception?
135+
*/
135136
try {
136137
currentThread.setName(threadName);
137138
return true;

android/guava/src/com/google/common/util/concurrent/MoreExecutors.java

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -739,15 +739,17 @@ public void run() {
739739
/**
740740
* Returns a default thread factory used to create new threads.
741741
*
742-
* <p>On AppEngine, returns {@code ThreadManager.currentRequestThreadFactory()}. Otherwise,
743-
* returns {@link Executors#defaultThreadFactory()}.
742+
* <p>When running on AppEngine with access to <a
743+
* href="https://cloud.google.com/appengine/docs/standard/java/javadoc/">AppEngine legacy
744+
* APIs</a>, this method returns {@code ThreadManager.currentRequestThreadFactory()}. Otherwise,
745+
* it returns {@link Executors#defaultThreadFactory()}.
744746
*
745747
* @since 14.0
746748
*/
747749
@Beta
748750
@GwtIncompatible // concurrency
749751
public static ThreadFactory platformThreadFactory() {
750-
if (!isAppEngine()) {
752+
if (!isAppEngineWithApiClasses()) {
751753
return Executors.defaultThreadFactory();
752754
}
753755
try {
@@ -772,10 +774,15 @@ public static ThreadFactory platformThreadFactory() {
772774
}
773775

774776
@GwtIncompatible // TODO
775-
private static boolean isAppEngine() {
777+
private static boolean isAppEngineWithApiClasses() {
776778
if (System.getProperty("com.google.appengine.runtime.environment") == null) {
777779
return false;
778780
}
781+
try {
782+
Class.forName("com.google.appengine.api.utils.SystemProperty");
783+
} catch (ClassNotFoundException e) {
784+
return false;
785+
}
779786
try {
780787
// If the current environment is null, we're not inside AppEngine.
781788
return Class.forName("com.google.apphosting.api.ApiProxy")
@@ -833,10 +840,6 @@ static Thread newThread(String name, Runnable runnable) {
833840
static Executor renamingDecorator(final Executor executor, final Supplier<String> nameSupplier) {
834841
checkNotNull(executor);
835842
checkNotNull(nameSupplier);
836-
if (isAppEngine()) {
837-
// AppEngine doesn't support thread renaming, so don't even try
838-
return executor;
839-
}
840843
return new Executor() {
841844
@Override
842845
public void execute(Runnable command) {
@@ -862,10 +865,6 @@ static ExecutorService renamingDecorator(
862865
final ExecutorService service, final Supplier<String> nameSupplier) {
863866
checkNotNull(service);
864867
checkNotNull(nameSupplier);
865-
if (isAppEngine()) {
866-
// AppEngine doesn't support thread renaming, so don't even try.
867-
return service;
868-
}
869868
return new WrappingExecutorService(service) {
870869
@Override
871870
protected <T> Callable<T> wrapTask(Callable<T> callable) {
@@ -896,10 +895,6 @@ static ScheduledExecutorService renamingDecorator(
896895
final ScheduledExecutorService service, final Supplier<String> nameSupplier) {
897896
checkNotNull(service);
898897
checkNotNull(nameSupplier);
899-
if (isAppEngine()) {
900-
// AppEngine doesn't support thread renaming, so don't even try.
901-
return service;
902-
}
903898
return new WrappingScheduledExecutorService(service) {
904899
@Override
905900
protected <T> Callable<T> wrapTask(Callable<T> callable) {

guava-gwt/test/com/google/common/util/concurrent/testModule.gwt.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
<inherits name="com.google.common.testing.Testing"/>
1010
<inherits name="com.google.common.util.concurrent.Concurrent"/>
1111
<inherits name="com.google.common.truth.Truth"/>
12+
<inherits name="activation.Activation"/>
1213
<entry-point class="com.google.common.util.concurrent.TestModuleEntryPoint"/>
1314

1415
<source path=""/>

guava/src/com/google/common/util/concurrent/Callables.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,10 @@ public void run() {
129129
/** Tries to set name of the given {@link Thread}, returns true if successful. */
130130
@GwtIncompatible // threads
131131
private static boolean trySetName(final String threadName, Thread currentThread) {
132-
// In AppEngine, this will always fail. Should we test for that explicitly using
133-
// MoreExecutors.isAppEngine? More generally, is there a way to see if we have the modifyThread
134-
// permission without catching an exception?
132+
/*
133+
* setName should usually succeed, but the security manager can prohibit it. Is there a way to
134+
* see if we have the modifyThread permission without catching an exception?
135+
*/
135136
try {
136137
currentThread.setName(threadName);
137138
return true;

guava/src/com/google/common/util/concurrent/MoreExecutors.java

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -814,15 +814,17 @@ public void run() {
814814
/**
815815
* Returns a default thread factory used to create new threads.
816816
*
817-
* <p>On AppEngine, returns {@code ThreadManager.currentRequestThreadFactory()}. Otherwise,
818-
* returns {@link Executors#defaultThreadFactory()}.
817+
* <p>When running on AppEngine with access to <a
818+
* href="https://cloud.google.com/appengine/docs/standard/java/javadoc/">AppEngine legacy
819+
* APIs</a>, this method returns {@code ThreadManager.currentRequestThreadFactory()}. Otherwise,
820+
* it returns {@link Executors#defaultThreadFactory()}.
819821
*
820822
* @since 14.0
821823
*/
822824
@Beta
823825
@GwtIncompatible // concurrency
824826
public static ThreadFactory platformThreadFactory() {
825-
if (!isAppEngine()) {
827+
if (!isAppEngineWithApiClasses()) {
826828
return Executors.defaultThreadFactory();
827829
}
828830
try {
@@ -847,10 +849,15 @@ public static ThreadFactory platformThreadFactory() {
847849
}
848850

849851
@GwtIncompatible // TODO
850-
private static boolean isAppEngine() {
852+
private static boolean isAppEngineWithApiClasses() {
851853
if (System.getProperty("com.google.appengine.runtime.environment") == null) {
852854
return false;
853855
}
856+
try {
857+
Class.forName("com.google.appengine.api.utils.SystemProperty");
858+
} catch (ClassNotFoundException e) {
859+
return false;
860+
}
854861
try {
855862
// If the current environment is null, we're not inside AppEngine.
856863
return Class.forName("com.google.apphosting.api.ApiProxy")
@@ -908,10 +915,6 @@ static Thread newThread(String name, Runnable runnable) {
908915
static Executor renamingDecorator(final Executor executor, final Supplier<String> nameSupplier) {
909916
checkNotNull(executor);
910917
checkNotNull(nameSupplier);
911-
if (isAppEngine()) {
912-
// AppEngine doesn't support thread renaming, so don't even try
913-
return executor;
914-
}
915918
return new Executor() {
916919
@Override
917920
public void execute(Runnable command) {
@@ -937,10 +940,6 @@ static ExecutorService renamingDecorator(
937940
final ExecutorService service, final Supplier<String> nameSupplier) {
938941
checkNotNull(service);
939942
checkNotNull(nameSupplier);
940-
if (isAppEngine()) {
941-
// AppEngine doesn't support thread renaming, so don't even try.
942-
return service;
943-
}
944943
return new WrappingExecutorService(service) {
945944
@Override
946945
protected <T> Callable<T> wrapTask(Callable<T> callable) {
@@ -971,10 +970,6 @@ static ScheduledExecutorService renamingDecorator(
971970
final ScheduledExecutorService service, final Supplier<String> nameSupplier) {
972971
checkNotNull(service);
973972
checkNotNull(nameSupplier);
974-
if (isAppEngine()) {
975-
// AppEngine doesn't support thread renaming, so don't even try.
976-
return service;
977-
}
978973
return new WrappingScheduledExecutorService(service) {
979974
@Override
980975
protected <T> Callable<T> wrapTask(Callable<T> callable) {

0 commit comments

Comments
 (0)