move ExecutionContext from Promise to app callbacks #16

wants to merge 4 commits into

4 participants


No description provided.

havocp added some commits May 2, 2012
@havocp havocp Some tests for how an implicit ExecutionContext is used by Future
These tests currently check that the implicit ExecutionContext
is not used by Future callback methods like onSuccess and map.
@havocp havocp Move implicit ExecutionContext to be determined by lexical scope
 - declare the invariant that all app callbacks have an
   associated ExecutionContext provided at the place
   the callback is passed to a method on Future
 - always run callbacks in their associated EC
 - since all callbacks have their own EC, Promise
   does not need one
 - "internal" callbacks don't need to defer execution either
   since we know the ultimate app callback will do so,
   therefore we can use an immediate executor for these
@viktorklang viktorklang and 2 others commented on an outdated diff May 2, 2012
trait Future[+T] extends Awaitable[T] {
+ // we run implementation-detail callbacks on this,
+ // they just run immediately, and actual deferral
+ // only happens when an application executor is
+ // provided along with an application callback.
+ // Having this implicit also ensures we never
+ // use ExecutionContext.defaultExecutionContext
+ // accidentally since this overrides it.
+ // The downside is that the methods with an
+ // application executor have an ambiguous
+ // executor.
viktorklang May 2, 2012

I don't see how this will fly. Wasn't your point that you wanted an application executor?

havocp May 2, 2012

The idea is that modulo bugs, this executor is only used in the lexical scope of the Future class, it's never used for the app's callbacks.

By "app" I just mean "code outside the Future implementation"

Another way to look at it is that if the app has any callbacks on the Future, there is always an app executor "after" this internal one (associated with the app callback). You can see later in the code that in methods that take an implicit executor from the app, that executor is manually used rather than this internal one.

So this internal EC should be an undetectable implementation detail. I tried a different way to do it (adding virtual methods to use rather than the existing onComplete) but I think this way is less code and functionally equivalent.

It also adds the elegance that the patch uses its own new feature (lexically-scoped custom executors) to implement itself - kind of fun!

phaller May 2, 2012 Owner

I don't think we need this executor (see below).

@viktorklang viktorklang and 1 other commented on an outdated diff May 2, 2012
@@ -628,6 +642,23 @@ object Future {
for (r <- fr; b <- fb) yield (r += b)
+ // This is used to run callbacks which we know are
+ // our own and not from the application; we can
+ // just run them immediately with no dispatch
+ // overhead, and we can know that they won't block,
+ // and we can know that exceptions from them are
+ // bugs in our stuff or maybe some nasty VM problem.
+ // Obviously don't use this to run a
+ // callback which in turn runs an application
+ // callback; only purely internal callbacks.
+ private[concurrent] object InternalCallbackExecutor extends ExecutionContext {
+ def execute(runnable: Runnable): Unit =
havocp May 2, 2012

The always sync or always async rule? I guess I'd say it is always sync in the lexical scope of Future and always async in the lexical scope of the app. (Again, app just means "not Future internals".)

Or more precisely, from the perspective of the app it's "always use the EC the app provided to run the app's callback", I'm assuming the app's EC is async, but anyway it's always whatever the app wanted. Modulo bugs there's no way an app's code ends up running in the internal executor AFAIK.

So I think everyone here has strong guarantees; Future knows that it will run its own code sync, and run the app's code how the app wanted (normally async, e.g. with the defaultExecutionContext).

@viktorklang viktorklang commented on the diff May 2, 2012
- def onFailure[U](callback: PartialFunction[Throwable, U]): this.type = onComplete {
+ def onFailure[U](callback: PartialFunction[Throwable, U])(implicit executor: ExecutionContext): this.type = onComplete {
viktorklang May 2, 2012

Alright, and what do with the Java-API (i.e. don't have to explicitly provide ec (when you don't have one))?

havocp May 2, 2012

Needs some thought.

Option 1: Java just passes extra parameter

I guess the default situation is that it's the same as the existing calls in Akka 2.0 that need a dispatcher, like:

Futures.successful(4, system.dispatcher());

So the solution space should be the same as it is for that. With this patch you wouldn't have to provide system.dispatcher() creating a Promise anymore but you would for onComplete(), etc. Typing , system.dispatcher() isn't that horrible given all the crap you already have to type to write an anonymous callback class in Java, but maybe we can do better.

Option 2: Differently-named version of each method using a default EC

From the Java perspective, there's no reason you couldn't have an overload with and without the executor, using a default executor for the one without. But of course that breaks Scala since the overloads would be distinguishable in bytecode but not in Scala source.

Simple if not 100% pretty solution: name the overloads different things,

onFailure(callback, executor)
onFailureDefaultExecutor(callback) // brainstorm better name

On the crazier side, maybe there's some language extension that would let us add an overload to the class file that Java would see and Scala would ignore; it looks like right now the generated class file from onFailure(callback)(executor) has an onFailure(callback, executor) with one combined parameter list. From Java we'd like to add plain onFailure(callback), but hide that from Scala. I can imagine some language feature to allow that, like @javaOnly or something. But clearly not a short-term solution.

Option 3: withDefaultExecutor()

Another possible option, just to expand the possibilities, might be to add:

// Future with an ExecutionContext associated with callbacks you add to it,
// intended for use from Java; not needed in Scala
trait FutureWithExecutor[T] {
    def future: Future[T]
    def onComplete(callback: Either[Throwable, T] => U): this.type

// existing Future trait gets new methods
trait Future[T] {
   def withExecutor(executor: ExecutionContext): FutureWithExecutor[T]
   def withDefaultExecutor: FutureWithExecutor[T]

So the idea would be that your Java code is like:


Where ... expands to the Java anonymous callback class.

I kind of like this solution since it avoids adding a lot of methods to Future itself that don't make sense in Scala, and it's more DRY in Java.

It does allow Java programmers to shoot themselves in the foot in the way this patch is intended to fix, if they start passing FutureWithExecutor across module boundaries instead of passing Future around, but they'll hopefully not do that. Anyway anytime they go through scala.concurrent or Akka APIs they'd have to convert back to regular Future.

Maybe we can think of more options, these are what I thought of so far.

havocp May 2, 2012

Here is option 3 in untested code form:

It's not mutually exclusive with Option 1 of course. I'm pretty ambivalent on whether adding 3 to 1 is worth the code, it sort of depends on how often Java developers chain Future methods.

Option 3 seems nicer than Option 2 to me.

axel22 May 11, 2012 Collaborator

Of these 3 options option no.2 seems most favorable to me. Here's why.

Imagine a Java client writes a couple of methods like this:

public FutureWithExecutor<T> jFlatMap(f1: FutureWithExecutor[T], f2: FutureWithExecutor[T]) {
  return f1..flatMap(f2);

public FutureWithExecutor<T> jForeach(f1: Future[T], f: Foreach[T, Unit]) {
  FutureWithExefcutor[T] f2 = f1.withDefaultExecutor();
  return f2;

and used within his Java code. He sets the return types to FutureWithExecutor because this way he avoids having to call withDefaultExecutor in every other method, like jFlatMap. Then a Scala client comes and calls a Java method jForeach:

implicit val ec = myEc
def scalaMethod(f: Future[T]) {
  jForeach(f, x => println(x)) map {
    y => println(y.hashCode)

One might expect that the map in the scalaMethod would pick up the implicit execution context in scope, but it does not, because jForeach returns a FutureWithExecutor instead of Future. This might result in subtle bugs which can be a pain.

For this reason I would be explicit in the Java code - rename these methods to something else.

axel22 May 11, 2012 Collaborator

Or simply require Java users to specify the additional parameter.

viktorklang May 11, 2012

Yeah, I think this is preferable actually.

havocp May 11, 2012

fwiw I would lean toward option 1 (require Java users to specify the extra param) as well. The other solutions don't seem like they pull their weight, they add a bunch of methods and some cognitive overhead for a pretty small gain in convenience.

viktorklang May 11, 2012

Yeah, that was exactly our consensus as well. Also, it's easier to solve a perceived problem than to retract a bad solution after the fact...

@phaller phaller commented on the diff May 2, 2012
case Left(t) => p failure t
case Right(v) => p success v
- }
+ })(internalExecutor)
phaller May 2, 2012 Owner

I think it would be better to also use executor here. It would be more regular to always complete promise p on the same EC. And I don't see a reason not to use executor. If executor is good enough to run f(v) and p complete resolver(t) on, then it should be OK to also just complete p on it.

havocp May 2, 2012

Here using internalExecutor is just an optimization, you're right it's fine to remove it here.

But internalExecutor doesn't exist because of this, this is just the one place where it's used when we already have an app executor. So it's also the one place it's specified explicitly instead of implicitly, because the app executor's presence makes the implicits ambiguous.

The reason internalExecutor exists is to cover all the spots where we call onComplete, onSuccess, etc. but don't have an app executor. Look for example at the implementation of def failed, but there are a lot of places we use it.

The pattern is almost always:

val p = Promise[T]

onComplete {
    // whatever


The internal executor is just running some quick internal code that converts Either[Throwable, T] to Throwable (for example in failed), or whatever thing like that. Then the app callback if any will have its own executor.

So in most places internalExecutor is not just an optimization, it's needed to call onComplete. The alternative I tried first was to add an onCompleteInternal onSuccessInternal etc. that don't take an executor, to be implemented by DefaultPromise, KeptPromise by running the callback immediately... but it's more code, and ends up being equivalent.

havocp May 2, 2012

Oh, worth mentioning, it would also be semantically correct to use defaultExecutionContext here, but since the internal executor is only 8 lines of code I figured why not avoid the overhead; there's no reason to dispatch to a thread pool when we know the app executor will dispatch to the app anyway. We know this internal executor is always just an intermediary.

havocp added some commits May 4, 2012
@havocp havocp Fix the docs on Future callbacks' ExecutionContext
They said the callback ran either immediately or in the
provided ExecutionContext, but really the callback is
guaranteed to be run in the ExecutionContext.
However ExecutionContext.execute() may be run either
immediately or asynchronously from some other ExecutionContext.
@havocp havocp improve the comments explaining InternalCallbackExecutor
Attempt to add some more clarity about why it exists and
why it is safe, for future readers of the code.

A couple related follow-on changes are in but I'm not adding them to this pull request (unless you want them here) since they are separable. The follow-on changes add the withDefaultExecutionContext for Java and tweak the _taskStack batching to avoid batching across executors. I'd send a new pull with those follow-on changes if you want them after we sort out this pull.

@viktorklang viktorklang commented on the diff May 9, 2012
@@ -106,22 +104,30 @@ object Promise {
}) match {
case null => false
case cs if cs.isEmpty => true
- case cs => Future.dispatchFuture(executor, () => cs.foreach(f => notifyCompleted(f, resolved))); true
+ // this assumes that bindDispatch() was called to create f,
+ // so it will go via dispatchFuture and notifyCompleted
+ case cs => cs.foreach(f => f(resolved)); true
viktorklang May 9, 2012

What happens if one of these throws an exception?

havocp May 9, 2012

The same thing that happened before, the dispatchFuture/notifyCompleted are still used and still catch exceptions. They are just moved below into bindDispatch(), i.e. the closure that used to be created here is created "in advance" because we need to capture the executor in onComplete rather than capturing it here.

(I reworked this a bit in the follow-on branch I mentioned in comments (not in this pull request yet), the rework may make this clearer but isn't intended to change the behavior. This patch makes the more minimal and straightforward change and avoids the potentially more controversial rework.)

viktorklang May 9, 2012

Ah, I was thrown off by the fuzziness of "this assumes ..."


Hey, thanks for merging! There is still an important bugfix in havocp@1c8fc6d

Should I do a pull vs. phaller/scala or scala/scala for that?


Yep, I'll review this.

@phaller phaller pushed a commit that referenced this pull request Nov 14, 2014
@retronym retronym SI-8933 Disable static Symbol literal cache in traits
In Scala 2.8.2, an optimization was added to create a static
cache for Symbol literals (ie, the results of `Symbol.apply("foo"))`.
This saves the map lookup on the second pass through code.

This actually was broken somewhere during the Scala 2.10 series,
after the addition of an overloaded `apply` method to `Symbol`.

The cache synthesis code was made aware of the overload and brought
back to working condition recently, in #3149.

However, this has uncovered a latent bug when the Symbol literals are
defined with traits.

One of the enclosed tests failed with:

	  jvm > t8933b-run.log
	java.lang.IllegalAccessError: tried to access field MotherClass.symbol$1 from class MixinWithSymbol$class
	        at MixinWithSymbol$class.symbolFromTrait(A.scala:3)
	        at MotherClass.symbolFromTrait(Test.scala:1)

This commit simply disables the optimization if we are in a trait.
Alternative fixes might be: a) make the static Symbol cache field
public / b) "mixin" the static symbol cache. Neither of these
seem worth the effort and risk for an already fairly situational

Here's how the optimization looks in a class:

	% cat sandbox/test.scala; qscalac sandbox/test.scala && echo ":javap C" | qscala;
	class C {
	  'a; 'b
	Welcome to Scala version 2.11.5-20141106-145558-aa558dce6d (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_20).
	Type in expressions to have them evaluated.
	Type :help for more information.

	scala> :javap C
	  Size 722 bytes
	  MD5 checksum 6bb00189166917254e8d40499ee7c887
	  Compiled from "test.scala"
	public class C

	  public static {};
	    descriptor: ()V
	      stack=2, locals=0, args_size=0
	         0: getstatic     #16                 // Field scala/Symbol$.MODULE$:Lscala/Symbol$;
	         3: ldc           #18                 // String a
	         5: invokevirtual #22                 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol;
	         8: putstatic     #26                 // Field symbol$1:Lscala/Symbol;
	        11: getstatic     #16                 // Field scala/Symbol$.MODULE$:Lscala/Symbol$;
	        14: ldc           #28                 // String b
	        16: invokevirtual #22                 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol;
	        19: putstatic     #31                 // Field symbol$2:Lscala/Symbol;
	        22: return

	  public C();
	    descriptor: ()V
	    flags: ACC_PUBLIC
	      stack=1, locals=1, args_size=1
	         0: aload_0
	         1: invokespecial #34                 // Method java/lang/Object."<init>":()V
	         4: getstatic     #26                 // Field symbol$1:Lscala/Symbol;
	         7: pop
	         8: getstatic     #31                 // Field symbol$2:Lscala/Symbol;
	        11: pop
	        12: return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment