UnsupportedOpperationException in toJava(future).toCompletableFuture is unnecessarily hostile (was: "Testing")

[jroper]

Most testing frameworks, in particular, the most popular Java Framework, junit, are synchronous, requiring that the test finish synchronously. When testing code that returns a CompletionStage, the only way to test this from junit is to block on it. And the idiomatic way, and simplest way, to do this is to call toCompletableFuture().get(). Since FutureConverters.toJava doesn't implement toCompletableFuture, this is going to make life difficult for Java developers when they want to test their code, especially given the unpredictable nature, when sometimes code might return a CompletableFuture, and so it would work, and other times, it might return a converted Scala Future, and then it won't work. To the end user, this will be confusing.

The spec for CompletionStage does not require that completing the return value of toCompletableFuture completes the original CompletionStage:

invocation of this method may be equivalent in effect to thenApply(x -> x), but returning an instance of type CompletableFuture.

It is therefore perfectly acceptable to return a new CompletableFuture that doesn't redeem the original CompletionStage, and for the sake of allowing Java developers to easily test APIs that return CompletionStage created by this library, I think this is very important.

[retronym]

/cc @rkuhn, who contributed this part of the library.

[jroper]

I just encountered this then when writing tests, I had the following function:

    private <T> T await(CompletionStage<T> cs) throws Exception {
        return cs.toCompletableFuture().get();
    }

And it worked for 6 tests. I then wrote a seventh test, and was surprised when it failed - nothing about my code indicated that it should fail on the seventh, the tests themselves didn't touch the scala-java8-compat library, it was all internals of how everything happened to be implemented.

The simplest way to fix this is actually to change the function to:

    private <T> T await(CompletionStage<T> cs) throws Exception {
        return cs.thenApply(Function.identity()).toCompletableFuture().get();
    }

If this behaviour isn't changed, then the very first stack overflow issue, Play issue, and mailing list message that will be create after Play 2.5 is released will be about this issue, and then Play users will have to remember to apply what should be a noop to their CompletionStages before they can block on them in their tests. I would be loathed to tell a user to do that, and then somehow still say that Play is a developer friendly framework.

[retronym]

Maybe it is just because I am unfamiliar with the new Java 8 concurency tools, but I am having a hard time parsing this bug report. What is your proposed change? What was the source of non-determinism in the first formulation of def await?

[rkuhn]

Well, we might not be able to change the world after all. I am not entirely certain but I think we prohibited toCompletableFuture precisely because it makes it easy to call get (which we don’t want). But of course that only makes sense once testing frameworks support asynchronous completion of test cases, like ScalaTest does. The easy route would be to just allow toCompletableFuture to succeed while the right way would be to (write and popularize a JDK or write and popularize an async JUnit or …). Normally I’d pick “right” over “easy”, but this time it might be adequate to chicken out. @viktorklang?

[viktorklang]

You rang?

I'm fine with toCompletableFuture returning a CompletableFuture that they can then use to block on.
I'm not fine with us encouraging them to do that.

Does that address the question?

[viktorklang]

Oh, and for testing:

I think we ought to introduce specific test methods that -always- require a timeout and whose names state very clearly that this is not to be done outside of tests. Like:

assertResultWithin(time, future, value)
assertFailureWithin(time, future, exception)

[jroper]

But - does it matter when testing, whether people block on futures or not? Writing asynchronous code is always a hit in complexity and readability, you do it out of necessity, not because you can do it. I see no necessity in tests to make code asynchronous, so therefore I would choose increased readability and simplicity over asynchrony, and make my tests blocking and synchronous.

And, is it really necessary to always require a timeout at the assertion level? Choosing such a timeout is completely arbitrary - we're always finding that we have to increase out timeouts in Play because our build infrastructure has problems and sometimes things that usually take milliseconds actually do legitimately take 10s of seconds due to running on a cloud shared with who knows what other services. The problem that we want to solve is that you want a timeout for your build. But this is already solved at the build level by Jenkins/Travis. Solving it at the assertion level is in my opinion the wrong granularity, and leads to the problems that we have where occasionally one test takes much longer than normal, causing false negatives.

[jroper]

@retronym, the problem is that the CompletionStage.toCompletableFuture provided by this library throws UnsupportedOperationException. The await methods from above are just examples of its use in my codebase.

[viktorklang]

And, is it really necessary to always require a timeout at the assertion level? Choosing such a timeout is completely arbitrary - we're always finding that we have to increase out timeouts in Play because our build infrastructure has problems and sometimes things that usually take milliseconds actually do legitimately take 10s of seconds due to running on a cloud shared with who knows what other services. The problem that we want to solve is that you want a timeout for your build. But this is already solved at the build level by Jenkins/Travis. Solving it at the assertion level is in my opinion the wrong granularity, and leads to the problems that we have where occasionally one test takes much longer than normal, causing false negatives.

The timeout should IMO always be present, even if it would use a finite, environment-appropriate, default if omitted. I see 0 reasons why you'd ever want an unbounded wait. Can you think of a situation where unbounded wait would be the only sensible value?

[jroper]

Can you think of a situation where unbounded wait would be the only sensible value?

Perhaps we should program like this:

val i = doWithTimeout(2.milliseconds, 10)
val j = doWithTimeout(2.milliseconds, 10)
val k = doWithTimeout(2.milliseconds, i + j)

Then I am ensuring that none of my operations are unbounded. In answer to your question, it's not about no timeout, it's about where you timeout. Timing out on every assertion is too fine grained, so I would do an unbounded wait there, and do the timeout at a higher level, eg, on the build.

[viktorklang]

I'd rather have a testcase time out and see which tests timed out than have the runner force-terminate the entire JVM. Especially for long-running test suites.

[jroper]

Sure, then let's do that, most test frameworks support configuring the timeout of test cases at the test case level, that would be the right level to time out at, it can also be easily configured globally, and can easily be turned off, for when, for example, you're running the tests with a debugger and spending half an hour inspecting the state of a single test case is the debugger. Timing out at the assertion level would be the wrong level, that's where I want to do an unbounded wait, and let the test framework handle my timeouts for me.

[viktorklang]

@jroper Sure, in my assertion example I'd only have a single invocation per test-case, and at the bottom of the test case, due to the lack of returning a CompletionStage. So that solves both of our concerns!

[He-Pin]

how about using a factor based on the current load via OperatingSystemMXBean?

[jroper]

Ok, so this got a bit derailed on whether there should be timeouts.

The problem with:

assertResultWithin(time, future, value)
assertFailureWithin(time, future, exception)

is that it doesn't allow running assertions on the value itself, other than a blanket equals, which is very often not suitable. We need to return the value so that assertions can be run on it.

Here's the thing, the horse has bolted on whether you should be able to block on futures in Java or not. We can't change that. When users block on one returned by Play or Akka, and find sometimes, under conditions that they don't understand, it works, and other times it doesn't, they will look up the problem on Google, find a stack overflow post with 1000 upvotes because so many people had this problem, see the top answer that says future.thenApply(Function.identity()).toCompletableFuture().get(), and copy and paste that. No matter what you do to make it hard to block on futures, you can't stop people from doing it. Every implementation, except this one, will allow users to block on it. Right now, by not allowing blocking on the futures converted by this library, we are punishing our end users for the bad design choice by the JDK designers, giving them surprising behaviour and excess boiler plate that they have to write. Some are blocking in the wrong places, but some know not to block in the wrong places, and are trying to do the right thing, just trying to write tests using a synchronous test framework. They don't deserve to be punished like that.

[rkuhn]

@jroper +1