eclipse / microprofile-reactive-streams-operators

Microprofile project
Apache License 2.0
80 stars 33 forks source link

DefaultCompletionSubscriber doesn't redeem CompletionStage #129

Closed danielkec closed 4 years ago

danielkec commented 4 years ago

Interface javadoc says

/**
 * A subscriber that redeems a completion stage when it completes.
 * <p>
 * The result is provided through a {@link CompletionStage}, which is redeemed when the subscriber receives a
 * completion or error signal, or otherwise cancels the stream.
 * <p>

But DefaultCompletionSubscriber does nothing with supplied CompletionStage in both cases:

            @Override
            public void onError(Throwable throwable) {
                subscriber.onError(throwable);
            }

            @Override
            public void onComplete() {
                subscriber.onComplete();
            }
akarnokd commented 4 years ago

What's even more odd is that no test fails because of this.

Azquelt commented 4 years ago

DefaultCompletionSubscriber really just holds both a Subscriber and a CompletionStage and delegates calls to the Subscriber.

It's up to the creator of the DefaultCompletionSubscriber to ensure that the CompletionStage passed into CompletionSubscriber.of is redeemed when the subscriber completes or terminates.

In the core implmentation, the CompletionStage is obtained from the call to engine.buildSubscriber.

As a side note, there's actually no way to manually redeem a CompletionStage via that interface. It's up to the CompletionStage implementation as to how it gets redeemed. Usually, the implementation is CompletableFuture and it's redeemed using the complete method.

danielkec commented 4 years ago

It does not seems right for it to be part of the API as it may lead to wrong conclusions. Maybe change the javadoc of the of method for something like: Create wrapper for already associated CompletionStage and Subscriber, to make it clear what is expected?

Azquelt commented 4 years ago

Yes, I think that makes sense.

Azquelt commented 4 years ago

I've opened #150 to make it clear what the caller's responsibilities are if you'd like to take a look @danielkec.