ListenableFutureAdapter hard-codes Executor

[GoogleCodeExporter]

Futures.compose cannot be used in google appengine as 
Futures$ListenableFutureAdapter hard-
codes its Executor to a cached thread pool.  Please adjust this.

    private static class ListenableFutureAdapter<T> extends ForwardingFuture<T>
      implements ListenableFuture<T> {

    private static final Executor adapterExecutor =
        java.util.concurrent.Executors.newCachedThreadPool(); <--- this is the problem

Original issue reported on code.google.com by adrian.f...@gmail.com on 19 Jan 2010 at 1:05

[GoogleCodeExporter]

How do you propose implementing this without a thread pool?

Original comment by fry@google.com on 26 Jan 2011 at 10:37

[GoogleCodeExporter]

mainly the idea is allowing one to specify an executor service.  for this to 
work correctly when the executorservice is a caller thread, I've had to do some 
oddness:

https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/co
ncurrent/Futures.java

Feel free to inquire or revise the design.  as soon as there's a hook in guava, 
the class above will be deleted.

Original comment by fernc...@gmail.com on 26 Jan 2011 at 11:01

[GoogleCodeExporter]

It looks to me like what you want is:

   http://guava-libraries.googlecode.com/svn/trunk/javadoc/com/google/common/util/concurrent/Futures.html#compose(com.google.common.util.concurrent.ListenableFuture, com.google.common.base.Function, java.util.concurrent.Executor)

Original comment by fry@google.com on 28 Jan 2011 at 3:54

• Changed state: Invalid

[GoogleCodeExporter]

It looks like compose no longer references ListenableFutureAdapter anymore.  
this is good!

However, ListenableFutureAdapter is still (as of r07) hard-coding an executor,

can you update this and change makeListenable, makeChecked to allows users to 
pass their own?

Original comment by fernc...@gmail.com on 28 Jan 2011 at 4:14

[GoogleCodeExporter]

sortof like this...

   public static <T> ListenableFuture<T> makeListenable(Future<T> future, ExecutorService executorService) {
      if (future instanceof ListenableFuture<?>) {
         return (ListenableFuture<T>) future;
      }
      return ListenableFutureAdapter.create(future, executorService);
   }

Original comment by adrian.f...@gmail.com on 28 Jan 2011 at 4:38

[GoogleCodeExporter]

Thanks for reporting this.  I didn't realize that AppEngine permitted Executors 
but not *arbitrary* Executors.  Do you happen to have a link to documentation 
of the precise policy here?

Here, though, let's see if we can sidestep this question:  Are you in a 
position to use ListenableFuture from the beginning?  Futures.makeListenable is 
an ugly hack that has proven to be unnecessary in internal Google code, and I'm 
hoping to get rid of it.  Usually it's possible for the code that creates the 
Future to do one of the following:

1) Create a ValueFuture (or other implementation of AbstractListenableFuture) 
and call set() or setException() on it.

2) Call new ListenableFutureTask(Callable), and pass the result to 
executor.execute(Runnable) (instead of calling submit(Callable)).  Internally 
we have a class that allows you to wrap an ExecutorService to make its submit 
methods do this automatically, but we haven't released it yet, partly because I 
haven't gotten around to makeListenable yet.

Would one of these be an acceptable option for you?

Original comment by cpov...@google.com on 1 Feb 2011 at 12:39

[GoogleCodeExporter]

Original comment by fry@google.com on 2 Feb 2011 at 8:18

• Changed state: Accepted
• Added labels: Type-Enhancement

[GoogleCodeExporter]

Original comment by kevinb@google.com on 13 Jul 2011 at 6:18

• Changed state: Triaged

[GoogleCodeExporter]

Original comment by fry@google.com on 10 Dec 2011 at 3:42

• Added labels: Package-Concurrent

[GoogleCodeExporter]

This doesn't appear to be an issue anymore...?

Original comment by wasserman.louis on 28 Dec 2011 at 10:36

[GoogleCodeExporter]

looks like the code is still firing up an executorservice on static init:

http://www.google.com/codesearch#UKMs0lhE9bg/trunk/src/com/google/common/util/co
ncurrent/Futures.java&q=ListenableFutureAdapter%20package:http://guava-libraries
\.googlecode\.com

maybe we can lazy do this?

Also, there is a makeListenable with an executorservice arg, but this is 
package private

Original comment by fernc...@gmail.com on 29 Dec 2011 at 6:30

[GoogleCodeExporter]

Code search is still pointing to SVN, I think -- I don't think 
ListenableFutureAdapter even exists anymore.  
http://code.google.com/p/guava-libraries/source/browse/#git%2Fguava%2Fsrc%2Fcom%
2Fgoogle%2Fcommon%2Futil%2Fconcurrent

Original comment by wasserman.louis on 31 Dec 2011 at 5:14

[GoogleCodeExporter]

Looking at comment 6, which I missed.  I think the idea looks sound, and open 
to options.  Main thing is that there are services that generate Futures 
without exposing an ExecutorService, in this case the async urlfetch service in 
GAE.  GAE doesnt prohibit Executors, just ones that spawn threads.  

There are three ways we can handle this in jclouds I can tell:
1. Stop exposing ListenableFuture on our interfaces and only return one when 
convenient.
2. Find a way to convert a Future into a ListenableFuture with a provided 
ExecutorService
3. Convince GAE to return ListenableFuture on async url fetch service

I haven't yet evaluated point 6, just restating things fwiw

Original comment by adrian.f...@gmail.com on 31 Dec 2011 at 5:58

[GoogleCodeExporter]

RE: comment 11: Good point: We'd need to put defaultAdapterExecutor in its own 
class to prevent initialization under App Engine.  (I initially misread the 
comment as suggesting that defaultAdapterExecutor was in the top-level Futures 
class, but this isn't what was meant.)

I remain nervous about allowing ListenableFutureAdapter to be used with a 
bounded executor.  If the executor has n threads and there are n+1 concurrent 
Futures, the n+1st will never complete until one of the others completes.  This 
could produce deadlocks.  Consider the cast of n=1:

// Note: listenInPoolThread is the new name of makeListenable.
ListenableFuture original = listenInPoolThread(createFuture());
return chain(original, function);

Now |original| is tying up the one pool thread.  Suppose that the underlying 
createFuture() uses another listenInPoolThread() Future internally.  Its 
ListenableFutureAdapter won't be scheduled until the original Future is 
completed, but the original Future can't complete until the internal Future's 
ListenableFutureAdapter is scheduled.

Of course, deadlocks can occur under most approaches to concurrency, and we 
live with that possibility.  Perhaps we can do the same with 
listenInPoolThread, especially since it's already something of a hack.  But the 
potential deadlock I described feels worse than the average deadlock, so I'd 
prefer to avoid it if possible.

http://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/comm
on/util/concurrent/JdkFutureAdapters.java?r=6dc1da63206320ccdb959ccbcc811ee2c1e2
cc2e#94

Original comment by cpov...@google.com on 5 Jan 2012 at 8:33

[GoogleCodeExporter]

Option 3 seems to be a pie-in-the-sky sort of thing, but it seems like the best 
long-term solution.  Can we at least poke at this?

Original comment by wasserman.louis on 5 Jan 2012 at 9:08

[GoogleCodeExporter]

option 3: agreed pie in sky, or cloud rather (hardy har har ;) )

comment 14: I agree wrt possible deadlocks.  However, our code that uses this 
basically uses it to create concurrent versions of Iterables.transform, etc. 
which have no interdependencies.  unhooking/lazy setting a default executor 
service removes a worse hack which is for us to copy/paste the class except 
that :P

Original comment by adrian.f...@gmail.com on 9 Jan 2012 at 6:10

[GoogleCodeExporter]

Well, the folks at Google might be able to poke at the folks who work on GAE.  
It's not impossible.

Original comment by wasserman.louis on 9 Jan 2012 at 6:45

[GoogleCodeExporter]

I suspect that the App Engine behavior is intentional.  Probably they don't 
want to take the chance that anything would execute in one of their internal 
threads -- hence, no callback-based asynchrony and no ListenableFuture.

I'm now tentatively leaning toward adding this simply because 
listenInPoolThread is already a hack accompanied by a warning, so why not make 
it a hackier hack with more warnings? :)  We'll need to think it over some 
more, but I'll tag it as Release12 so that we're forced to come back to it 
before then.

Original comment by cpov...@google.com on 12 Jan 2012 at 9:35

• Added labels: Milestone-Release12

[GoogleCodeExporter]

muchos gracias

Original comment by fernc...@gmail.com on 12 Jan 2012 at 9:39

[GoogleCodeExporter]

Original comment by fry@google.com on 16 Feb 2012 at 7:17

• Changed state: Acknowledged

[GoogleCodeExporter]

Original comment by cpov...@google.com on 23 Mar 2012 at 8:54

• Changed state: Started

[GoogleCodeExporter]

http://code.google.com/p/guava-libraries/source/detail?r=2c5657b1ee65c4f8338d1d9
b1ed595c4ab50c8fc

Original comment by cpov...@google.com on 24 Mar 2012 at 3:15

• Changed state: Fixed

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:16

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:10

• Added labels: Restrict-AddIssueComment-Commit