Reactor-core: StepVerifier thenCancel() cause false-pass if event emitted from other thread

[mpapirkovskyy]

Expected behavior

thenCancel() event should trigger after expectMatches() completes

Actual behavior

thenCancel() is triggered before expectMatches() completes even if it fails, so verify() may incorrectly complete without errors.
thenCancel() is triggered correctly if prepended by any then().

Steps to reproduce

JUnit tests for demonstration:
https://gist.github.com/mpapirkovskyy/8938b7f2423b0e6d40866a76f8e41f21

Reactor Core version

3.1.2.RELEASE

Details

Some investigation showed that incorrect behavior is caused by immediate reaction on SubscriptionEvent in script event if previous SingalEvent is still in progress:
https://github.com/reactor/reactor-core/blob/75a0ab0574aa5addc3f5da85247019dd81bc7393/reactor-test/src/main/java/reactor/test/DefaultStepVerifierBuilder.java#L1499-L1504
working case is result of wrapping SubscriptionEvent into SubscriptionTaskEvent when prepended by another TaskEvent, so it is added to queue explicitly after SignalEvent completes
https://github.com/reactor/reactor-core/blob/75a0ab0574aa5addc3f5da85247019dd81bc7393/reactor-test/src/main/java/reactor/test/DefaultStepVerifierBuilder.java#L846-L850

I cannot get reason why SubscriptionEvent is not always wrapped into SubscriptionTaskEvent.

[simonbasle]

The reason why is that thenCancel attempts to be as eager as possible, to avoid some never-ending sources hanging the tests (eg. in case of under-requesting, badly behaved sources, etc...).

However, I think it makes sense to add a convenience cancel method that ensures the previous expectation is processed, with a clear warning that it could lead to tests hanging. See associated PR

[Kindrat]

To my understanding - there is no single argument for verification library - e.g. assertJ to skip assertions in favor of 'never hanging tests'... you can always write test in the wrong way and it would take enormous time to complete.
To solve tests' deadlocks you should use test timeout in JUnit, TestNG, etc. Or global build timeout on CI, but not the implicit assertion lib behavior... By the way, reactor has the ability to interrupt long running chains by providing Duration in verify. Sorry, but it was really a headake to find this skip...
As for me - it would be more obvious to use 'no skip' approach as default and not add additional methods as it adds complexity without any reasonable profit.

[simonbasle]

I see your point, although cancellation of an asynchronous flow doesn't really bear comparison with a simple imperative assertion... It is more similar to a test where you'd schedule a Runnable on an ExecutorService which runs an assertion, and then never wait on the resulting Future and complaining that the test passes despite the assertion being wrong... Writing unit tests for asynchronous code probably needs a little more careful planning. Same here with the asynchronous cancellation, which can compete with an asynchronous emission (and this is _the only case you need to watch for_)...

Making the change breaks 66 reactor-core tests, so it's bound to similarly break a lot of tests for our users... we simply won't do that, at least not in any 3.1.x release.

Proposed PR doesn't change the behavior but makes it more explicit and gives you more tools and more options to craft the right unit test. The alternative would be to deprecate thenCancel, come up with a better name and introduce a breaking change in 3.2.

@smaldini @rstoyanchev any thought?

[mpapirkovskyy]

@simonbasle but current behavior is inconsistent.
.expect().thenCancel()
and
.expect().then(() -> noop()).thenCancel()
are completely different.
Also how tests can rely on skipping expect() ?

[Kindrat]

@mpapirkovskyy
Agree, It's not correct to compare with executor services in that way. @simonbasle imagine you have such test snippet:

    @org.junit.Test
    public void checkEventIsPublished() throws Exception {
        LinkedBlockingQueue<String> eventQueue = new LinkedBlockingQueue<>();
        ExecutorService executorService = Executors.newSingleThreadExecutor();
        String event = "test";

        executorService.submit(() -> eventQueue.add(event));
        assertThat(eventQueue.poll(10, TimeUnit.SECONDS)).isEqualTo(event);
    }

You claim It's ok to skip waiting for 10 sec b/c it's async operation? It's a common case when you have some source of 'random' events, but you know this source is publishing from multiple threads and every X seconds emits event of certain type (let's say ping). You want to check that flux will get this event in at least 2X time from those X seconds. But, suddenly, the test you've written is skipping verification without failing the test itself. If you miss this issue - it could get whole way to prod, etc...

I would rather recommend fix it, b/c I believe, it could save all your users also. Their green tests could actually hide problems.

[simonbasle]

in your example you're polling for up to 10 seconds on a blocking queue, which is not comparable to what I was describing: in StepVerifier the "assertions" are also run asynchronously, so the assert would be submitted to the executor too, and thenCancel would translate to shutting down the ExecutorService.

but I tend to agree that this is probably too misleading, we'll see what we can do to limit the damage in 3.1.3 and further fix in 3.2. I'm thinking:

• deprecate thenCancel, and alias it to something like "cancelImmediately" for the current behavior
  
  
  
  • add a "cancelAfterExpect" or something, to be recommended as the default for your usecase
  
  
  • remove deprecated thenCancel in 3.2
  
  

[smaldini]

Reworking the current open PR, I think we only want one behavior which is waiting sequentially for expectations to run. There was an oversight in short circuiting expectations probably due to history and initial StepVerifier impl. Note that we could broadly review the StepVerifier impl for 3.2 as it has grown a bit since 3.0 and could do with some simplifications 👍