servicetalk icon indicating copy to clipboard operation
servicetalk copied to clipboard

StepVerifier: *LastStep issues with cancellation and terminal events

Open idelpivnitskiy opened this issue 3 years ago • 0 comments

thenCancel() is on the *LastStep interface, but it triggers cancellation right away ignoring all previous steps.

Example:

    @Test
    public void requesterPayloadBodyDoesNotTimeoutWhenIgnored() {
        Duration timeout = ofMillis(100L);
        TestPublisher<Buffer> payloadBody = new TestPublisher<>();
        FilterableStreamingHttpConnection connection = connection(succeeded(newResponse(payloadBody)));
        StreamingHttpRequester requester = new TimeoutHttpRequesterFilter(timeout, false).create(connection);
        AtomicBoolean responseSucceeded = new AtomicBoolean();
        StepVerifiers.create(requester.request(STRATEGY, mock(StreamingHttpRequest.class))
                .whenOnSuccess(__ -> responseSucceeded.set(true))
                .whenCancel(() -> System.out.println("CANCELLED"))
                .flatMapPublisher(StreamingHttpResponse::payloadBody))
                .thenRequest(MAX_VALUE)
                .expectNoSignals(timeout.plusMillis(10L))
                .thenCancel()
                .verify();
        assertThat("Response did not succeeded", responseSucceeded.get(), is(true));
        assertThat("No subscribe for payload body", payloadBody.isSubscribed(), is(true));
    }

This test is expected to cancel after "expectNoSignals", but it cancels immediately.

The workaround is to intercept the Cancellable manually, schedule cancellation using executor, and use another terminal state. Example:

AtomicReference<Cancellable> cancellable = new AtomicReference<>();
stepVerifier(requester.request(...)
        .expectCancellableConsumed(cancellable::set)
        .then(() -> {
            try {
                secondRequestReceivedLatch.await();
                cancellable.get().cancel(); // If I use thenCancel() the current then(Runnable) does not run
            } catch (InterruptedException e) {
                // ignore
            }
        })
        .expectError()    // subscrived.onError(…) was never invoked, test is green
        .verify();

This is confusing because expectError is just ignored. It's only there to transition to verify() method through the last step.

thenCancel() should trigger cancellation only after all previous steps complete. Or we should have another step method that will have described behavior.

idelpivnitskiy avatar Apr 15 '21 18:04 idelpivnitskiy