Make Fail Overflow Test less flaky

[abutch3r]

Fail overflow test can fail to pass if exception is thrown after more then 256 messages have been processed.

increase window within which a failure can occur by applying a similar change as was done in #158

[eclipse-microprofile-bot]

Can one of the admins verify this patch?

[Azquelt]

@eclipse-microprofile-bot please test this

[Azquelt]

To be honest, this test looks pretty sketchy. We're submitting to the executor in a tight loop, but the methods downstream of the processor seem to also run as quickly as possible.

I don't think there's any guarantee in this test that the emitter will overflow?

[cescoffier]

Yes, it may not overflow depending on the timing of the test.

We may want to disable it completely.

[Azquelt]

@eclipse-microprofile-bot test this please

[abutch3r]

@cescoffier
I have updated the test bean to be in line with the other beans which all contain a 1 millisecond sleep, which should for all practical purposes mean that the test will now throw the failure as expected.

[Azquelt]

I think this looks good.

[Azquelt]

1 second sleep

That'll be a 1 millisecond sleep - which should still be enough to ensure that we send messages to the emitter faster than they can be processed.

[kabir]

We're also seeing occasional failures in DropOverflowStrategyOverflowTest/

https://issues.redhat.com/browse/WFLY-17635

Would a similar fix be appropriate there? I've not looked beyond verifying that the classes are different :-)

[Emily-Jiang]

We're also seeing occasional failures in DropOverflowStrategyOverflowTest/

https://issues.redhat.com/browse/WFLY-17635

Would a similar fix be appropriate there? I've not looked beyond verifying that the classes are different :-)

Please open a different issue and propose a PR @kabir

[Emily-Jiang]

@cescoffier are you okay with this PR?

[cescoffier]

@ozangunalp can you check? It should be fine (as discussed).

[ozangunalp]

It seems to me like it'll almost always fail.

The second emit will be called before the very first message arrives to the out because of the sleep(1), and cause a lack of requests exception without any messages registered at the output.

I think we should just get rid of the assertion assertThat(bean.output()).isNotEmpty().... The emission and processing happens on different threads, We can't be sure if we process any messages.

[abutch3r]

@kabir your issue matches the fix in #158 which is not currently in a published version of the TCK. When a new version is published then the fix for that test would also be included

[Azquelt]

It seems to me like it'll almost always fail.

The second emit will be called before the very first message arrives to the out because of the sleep(1), and cause a lack of requests exception without any messages registered at the output.

I think we should just get rid of the assertion assertThat(bean.output()).isNotEmpty().... The emission and processing happens on different threads, We can't be sure if we process any messages.

Good point.

In practise, the await() line may often wait long enough for the first message to arrive since it only checks the condition every 100ms by default, but it is still a nasty race condition.

[Azquelt]

Ok, @abutch3r and I just had a chat about this test.

As soon as the emitter overflows, it emits a failure and terminates the subscription (1.6). At this point all further messages should result in the Emitter throwing IllegalStateException because the stream is terminated.

So when the emitter first throws an exception, neither the first message, nor the failure signal may have reached the end of the stream yet.

Currently the test awaits on an exception being thrown from the emitter. We think if we change it to wait for the onError signal to arrive, that would guarantee that the assertThat(bean.output()).isNotEmpty().hasSizeLessThan(999); check would pass, since the first item should be processed before the error signal.

At that point, it would be theoretically possible for the test thread not to yet see an exception thrown from the emitter since that's done asynchronously, so we should await() on that arriving too.

[Azquelt]

This line isn't needed because emitOne runs synchronously.

[ozangunalp]

I am trying to find a way to make this test work but even by waiting on the failure() and not the exception(), the error signal will arrive first and complete the stream before the first message arrives to the downstream out channel.

[Azquelt]

the error signal will arrive first and complete the stream before the first message arrives to the downstream out channel

Hmm, I was under the impression that the failure signal should follow the items down the stream, not reaching the final subscriber until the previous two items had been processed, but I guess that's not the case?

[Emily-Jiang]

@cescoffier @ozangunalp do you have any objections to merge this PR?

[ozangunalp]

I think the previous changes are making this test even more flaky.
I suggest we keep the old assertion about the size of the output and make the other changes :

    @Test
    public void testOverflow() {
        bean.emitALotOfItems();

        await().until(() -> bean.failure() != null);
        assertThat(bean.failure()).isInstanceOf(Exception.class);
        assertThat(bean.output()).doesNotContain("999");
        assertThat(bean.output()).hasSizeBetween(0, 256);
        // If an exception has not yet been thrown after the failure occurred, try one more message
        if (bean.exception() == null) {
            bean.emitOne();
        }
        assertThat(bean.exception()).isInstanceOf(IllegalStateException.class);
    }

I think we can also revert the added Thread.sleep(1).

[Azquelt]

I think you need the Thread.sleep(1) to ensure that each message isn't just processed as it comes in.

Each call to emitter.send() results in an async task starting, and the test relies on the next call to emitter.send() running before the async task has completed. Though this is likely to happen for at least one of the 1000 calls, the Thread.sleep(1) almost guarantees it.

Having a lock in there so that the test can block tasks from completing until the emitter fails would be an alternative.

[abutch3r]

@ozangunalp

I have removed the .isNotEmpty() check from the assertions and reintroduced the .doesNotContain("999") as that validates that no messages are later accepted.

the main issue we saw with the test is that the failure could occur after more then 500 messages were processed, so the 256 check does need to be increased to allow for the situation where we do process more then expected, but the sleep which is used in every other emitter test should provide a better guarantee that we see it fail.

Are these changes now satisfactory

[ozangunalp]

Yes, this looks good to me, thanks! @abutch3r @Azquelt