Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specific Try (and Future) methods should take Checked functions #865

Closed
danieldietrich opened this issue Nov 26, 2015 · 14 comments
Closed

Comments

@danieldietrich
Copy link
Contributor

The original question was: "What about renaming Try.recoverWith to tryRecover which takes a CheckedFunction?"

It is important that it is consistent. I started to think about Function vs CheckedFunction args in Try methods when implementing common interfaces like Value. There we have map, flatMap but also orElseGet, orElseThrow et. al. These use Function. We expect all Values to behave the same.
We could make all Try-specific methods take Checked-versions.

This is also the reason why we have mapTry and flatMapTry. They can't overwrite the Value.map and Value.flatMap, which is a pitty. Similar cases we have with Map.map(BiFunction) and Map.flatMap(BiFunction) - they are special.

Q: Why do we need function anyway? Why not always a CheckedFuntion?

A: It is a matter of interoperability with Java 8. We need interfaces to the standard lib - otherwise Javaslang will be an Alien living on another Planet or the code gets too verbose because of the need to convert things...

Q: What about changing recoverWith to recoverTry?

A: I will take it into account. Deeper integration of Try outside of it / within other types might make sense. But we should keep the API thin but deep. In other words we should not create many words in our language for the same thing but compose our sentences from a set of few words.

Future has also Try capabilities. Maybe Try and Future need a common interface.

@RobWin
Copy link
Contributor

RobWin commented Dec 9, 2015

Try has a CheckedFunction interface, but Try.mapTry() takes a CheckedFunction1.
This seems to be inconsistent.

@danieldietrich
Copy link
Contributor Author

Thx, right! Will take that into account...

@RobWin
Copy link
Contributor

RobWin commented Dec 10, 2015

Can we delete the CheckedFunction in Try? But then its confusing to have CheckedSupplier, CheckedConsumer in a different package than CheckedFunction1. Maybe its better to move them in the common root package?

@danieldietrich
Copy link
Contributor Author

We will never get the perfect solution with Java's functional interfaces - there is no 'one size fits all'. Scala has real function literals - a x -> yis always the same.

For Java things behave a little different. x -> y fits every @FunctionalInterface which has a method with argument x and return type y, even if we have a Consumer! Then the return type is discarded.

If we would move all functional interfaces to the root package, no one will know which to take. Therefore we do place the functional interfaces where they belong to (high cohesion principle).

That would also be my take when designing a new Java 8 application. Create functional interfaces where there belong. They need to be bound by 'business need'. Even if there are technically n types of function.

The Function1..8 / CheckedFunction1..8 are general purpose functions with rich functionality. Programmers can use them everywhere, if needed.

@RobWin
Copy link
Contributor

RobWin commented Dec 11, 2015

But there is not need for a CheckedFunction and a CheckedFunction1?
And why should a CheckedConsumer, CheckedPredicate and CheckedRunnable have a higher cohesion to Try than to other Monads. I think it would make sense to put all @FunctionalInterfaces into the same package like in Java java.util.function.

@danieldietrich
Copy link
Contributor Author

It is an aggregate dependency. If Try disappears, the Try.Checked* functions are not needed any more. They belong to each other. We need to model this fact with the possibilities we have.

On the other hand Also the packages of the Java lib are not state of the art. cough util cough.

@danieldietrich
Copy link
Contributor Author

Maybe you are right an we can remove all Try.Checked* interfaces.

Predicate, Runnable and Consumer are needed then, maybe with different names?

Supplier is Function0... will users recognize that?

If we provide these new functions people will ask for version with two or more arguments, maybe primitive data types... that would be too much.

I had all this and the code generator and the number of interfaces exploded (thousands).

This leads to the solution to separate the concerns and make a clear slicing.

javaslang.Function* are designed to existin a word where all is an object. Even Void return type.

@danieldietrich
Copy link
Contributor Author

@RobWin I will take this ticket. With #1044 all changes. We will not need the filterTry, ... methods any more and just have filter(CheckPredicate) instead of filterTry(CheckedPredicate). This will be awesome!

@danieldietrich
Copy link
Contributor Author

TODO:

  • onComplete, onSuccess and onFailure should take a Consumer (instead of a CheckedConsumer)
  • Try.andThen behaves different for CheckedRunnable and CheckedConsumer!?

@danieldietrich
Copy link
Contributor Author

The different andThen implementations of Java 8 - just for comparison.

Consumer.andThen:

default Consumer<T> andThen(Consumer<? super T> after) {
    Objects.requireNonNull(after);
    return (T t) -> { accept(t); after.accept(t); };
}

Function.andThen:

default <V> Function<T, V> andThen(Function<? super R, ? extends V> after) {
    Objects.requireNonNull(after);
    return (T t) -> after.apply(apply(t));
}

@danieldietrich
Copy link
Contributor Author

Chaining of Try/Future onSuccess & onFailure handlers is dangerous. Let's consider the following:

Try.of(...)
   .onSuccess(valueConsumer)
   .onFailure(throwableConsumer);

The onSuccess method returns the original Try.of(...) result, even if valueConsumer fails in the onSuccess method. This is important because otherwise we have no control when errors are handled.

Because of this the above consumers are not checked. If the consumers may throw, they should be handled as follows:

Try.of(...)
   .onSuccess(t -> Try.run(() -> valueConsumer.accept(t)))
   .onFailure(t -> Try.run(() -> throwableConsumer(t)));

@danieldietrich
Copy link
Contributor Author

Scala does the following (using import scala.concurrent.ExecutionContext.Implicits.global):

scala> val f = Future(1); f.onComplete(_ => throw new Error("test")); f.onComplete(t => println(t));
java.lang.Error: test
    at $line13.$read$$iw$$iw$$anonfun$2.apply(<console>:12)
    at $line13.$read$$iw$$iw$$anonfun$2.apply(<console>:12)
Success(1)
    at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:32)
    at scala.concurrent.impl.ExecutionContextImpl$AdaptedForkJoinTask.exec(ExecutionContextImpl.scala:121)
    at scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
    at scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
f: scala.concurrent.Future[Int] = scala.concurrent.impl.Promise$DefaultPromise@7c211fd0
    at scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
    at scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)

This means the second onComplete is executed because the error was thrown on a different thread.

Our Future.onComplete() method should behave the same.

@danieldietrich
Copy link
Contributor Author

While testing I changed the default executor service of Future to single-threaded. I think there is no 'best practice' which concurrent executor service to take (fixed size, scheduled, fork/join, pooled, etc.)

    /**
     * The default executor service is {@link Executors#newSingleThreadExecutor()}.
     * Outer executor services like {@link Executors#newCachedThreadPool()} may prevent the VM from exiting.}
     */
    ExecutorService DEFAULT_EXECUTOR_SERVICE = Executors.newSingleThreadExecutor();

@danieldietrich
Copy link
Contributor Author

Needed to switch back to CachedThreadPool because otherwise deadlocks may occur.

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

No branches or pull requests

2 participants