Shouldn't use toCompletableFuture in Combinators

[rnarubin]

Porting issue 36 from the enterprise repo

rkhadiwa commented on Nov 29, 2017

From CompletionStage javadoc, toCompletableFuture is optional.

     * A CompletionStage
     * implementation that does not choose to interoperate with others
     * may throw {@code UnsupportedOperationException}.
     *
     * @return the CompletableFuture
     * @throws UnsupportedOperationException if this implementation
     * does not interoperate with CompletableFuture

  public static CompletionStage<Void> allOf(
      final Collection<? extends CompletionStage<?>> stages) {

    final Iterator<? extends CompletionStage<?>> backingIt = stages.iterator();
    final int size = stages.size();
    final Iterator<? extends CompletionStage<?>> it =
        size > MAX_DEPENDANT_DEPTH
            ? new Iterator<CompletionStage<?>>() {
              @Override
              public boolean hasNext() {
                return backingIt.hasNext();
              }

              @Override
              public CompletionStage<?> next() {
                return backingIt.next().toCompletableFuture();
              }
            }
            : backingIt;
    return allOfImpl(it);
  }

Just a rant, toCompletableFuture is so stupid. Because it exists, CompletionStage isn't actually read only since CompletableFuture.toCompletableFuture returns this (and worse than a user completing a stage, the user can also use obtrude*). But because it's optional you can't depend on it for what little use it actually did have (interop).

[rnarubin]

rkhadiwa commented on Nov 29, 2017

Hm, apparently, in the Java9 javadoc they made toCompletableFuture mandatory

    /**
     * Returns a {@link CompletableFuture} maintaining the same
     * completion properties as this stage. If this stage is already a
     * CompletableFuture, this method may return this stage itself.
     * Otherwise, invocation of this method may be equivalent in
     * effect to {@code thenApply(x -> x)}, but returning an instance
     * of type {@code CompletableFuture}.
     *
     * @return the CompletableFuture
     */

is that grounds to close this issue, even though this is a Java 8 library?

[rnarubin]

rnarubin commented on Dec 11, 2017

wow i never thought about the repercussions of obtrude. that sucks too...

I think that in principle we need to consider java 8 as our foundation, and largely ignore java 9 (except for substantial and heavily related changes like the Flow API). That means we can't rely on toCompletableFuture, and for Combinators i think that means increasing MAX_DEPENDANT_DEPTH and then going with one of our previous convoluted combine implementations when exceeding that depth

[rnarubin]

rkhadiwa commented on Dec 11, 2017

I was more suggesting the change indicates that the documentation in Java8 was a bug. (Because clearly they realized how silly it was)

OTOH I think this is pretty much the only place that uses it, so I could live with supporting it.