Prevent unnecessary duplicated lines in backtraces

[simonbasle]

todo:

• rework the reference guide accordingly

outdated description:

This commit removes the `i` counter that was effectively guaranteeing
duplicated entries in the backtrace (eg. with a checkpoint) in the case
where the triggering exception was reused. The only differentiator was
then the indentation (driven by `i`).

This is especially visible when using a `checkpoint(String)` operator
along a `retry`. Provided the same exception instance is emitted on each
retry loop, `checkpoint` will add to the backtrace on each loop.

This commit removes the `i` from the tracking of existing backtraces and
thus ensures that for the same parent subscriber, prefix and description
there will be only one entry in the backtrace.

Fixes #2774.

[simonbasle]

cc @osi @smaldini

[osi]

intriguing that this is related to:

in the case where the triggering exception was reused.

.. its a useful clue to look in my code to see where i may be caching something that's unexpected!

[osi]

So I had a logic error in my code. I was retrying a Flux that was using cache deeper in the flow, making the retry useless.

As a pattern, this is bad, as retry won't ever succeed. The check against i is the signal that the same exception is being seen.

The current implementation failed by filling my disk with the repeated checkpoints.

What if the check for the same exception instance was kept, but instead of appending to the message it modified the message to indicate that it was repeated?

[simonbasle]

What if the check for the same exception instance was kept, but instead of appending to the message it modified the message to indicate that it was repeated?

I've thought about that but that implies more complex logic for tracking.

What's worse is that even simpler cases quickly become tricky to represent: for example, just creating 2 chains out of the same (erroring) source:

Flux<String> source = Flux.error(new RuntimeException("shared"));
Flux<String> chain1 = source.map(s -> s.toLowerCase());
Flux<String> chain2 = source.filter(s -> s.isEmpty());

This confuses the backtrace production, which would display both map and filter as if they were chained together.

Deep down, it's a matter of trying to represent a graph as a single sequence / line... 🤯

[simonbasle]

@osi @smaldini I have now something that displays a counter before the operator name. It still is not perfect from the perspective of displaying multiple paths, but at least it will prevent infinitely growing backtraces and repetitions while hinting that there are more complex codepaths in play.

[simonbasle]

I wonder to what extent it makes sense to target 3.3.x with this one. it is a change in behavior after all.

[simonbasle]

Note: The reactor-tools shouldn't be impacted, as it delegates to Traces and correctly detects checkpoints, and this change impacts what happens after when the backtrace message is generated.

[osi]

thank you @simonbasle

but at least it will prevent infinitely growing backtraces and repetitions while hinting that there are more complex codepaths in play.

this is perfect. it removes the opportunity for accidental catastrophe while leaving hints

[simonbasle]

I have devoted a bit more time to this, and ended up refactoring the "error has been observed" part of the traceback:

1. when multiple path are traversed, they are more obvious
2. turned (xN) into (observed N times)
3. make "roots" more obvious with *__ connector

It looks like this:

Assembly trace from producer [reactor.core.publisher.MonoDefer] :
	reactor.core.publisher.Mono.defer(Mono.java:213)
	reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:747)
Error has been observed at the following site(s):
	*____Mono.defer ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:747)
	*_____Flux.then ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:747)
	|_    Mono.flux ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:749)
	|_ Flux.publish ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:749) (observed 2 times)
	|_     Flux.map ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:751)
	*_____Flux.then ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:747)
	|_    Mono.flux ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:749)
	|_ Flux.publish ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:749) (observed 2 times)
	|_  Flux.filter ⇢ at reactor.core.publisher.HooksTest.testMultiReceiver(HooksTest.java:761)

(in the above we see 3 "roots", and 2 different sub-chains are better identified)

[simonbasle]

Note: fixes #2774 and #2785

[OlegDokuka]

LGTM