Proposed improvement on OnNextValue stacktrace rendition

[simonbasle]

Hi,
I was looking at a stacktrace with OnNextValue in it and saw that the last emitted item, causing the Exception, was indeed logged in the stacktrace as just a className, as per #1401. (see also the rendering code).

But in some contexts, like for the Couchbase SDK or some other framework using Rx, I think it is acceptable to say that we can identify more types that it makes sense to stringify. We can check that these types have a sufficiently performant, safe and slim toString() implementation to be fully rendered by OnNextValue.

Offering a mean to let RxJava know about such types would allow the String rendering to be activated in a cross-cutting manner, without reworking every single stream produced by the framework/library to induce additional logging behavior.

I see two broad ways of achieving such a thing:

• via an interface in RxJava
  • either a marker interface (implementing classes would then be rendered by calling their toString())
  • or a functional interface (implementing classes would have to define some sort of short, performant and safe toString(), toShortString()?, to be used by OnNextValue for rendering)
• achieve the same via a RxJava Plugin: providing a hook to decide or not if a value should be fully rendered (additional method on RxJavaErrorHandler?)

What do you think about this idea? If we can agree on a way of doing this, I'd be happy to contribute it!

[benjchristensen]

I'm okay with exploring improvements to this (and anything else to improve debugging).

The marker interface could work, but it is intrusive to the public API and the types being used by libraries. It seems the plugin model would be the most powerful and would work on any types without forcing them to conform to the RxJava marker interface, which is important if the types are 3rd party and can't be changed, or they live somewhere that should not have dependency on RxJava.

One drawback of how the plugin registration works right now is that it assumes and enforces only 1 registration at startup, so if 2 libraries both tried, one would fail. Perhaps for this use case we want to allow many plugin registrations?

It could be that RxJavaErrorHandler should support a chain of implementations?

@ReactiveX/rxjava-committers Do you have preferences on how this is pursued?

[akarnokd]

I don't want another Serializable so I'd go for the plugin-based callbacks. I'd extend RxJavaErrorHandler with a

public <T> Subscription registerErrorValueRenderer(
    Class<T> clazz, Func1<? super T, String> tostring);
public <T> boolean hasErrorValueRenderer(Class<T> clazz);

But what should happen if the same class was registered multiple times? I'd throw an IllegalArgumentException so libraries don't try to register for common types such as Integer but only their own types.

[simonbasle]

I was thinking about doing it the plugin way on the existing plugin but that breaks a committed interface doesn't it? So thinking about doing a separate plugin, and one that supports chaining (composite pattern?) :)

[akarnokd]

RxJavaErrorHandler is an abstract class so adding a regular method has only the risk if there is someone implementing this class with a method having the same name.

Chaining has some issues: in worst case it would take N steps to reach a value renderer. You'd need some cooperation mechanisms to see who is interested in giving an answer and not ask anyone else (such as Swing's KeyEvent.consume() or something).

[simonbasle]

Ah right it's abstract!
What I was going for was one-in-all renderer, something like:

public interface ErrorRenderer {
    String attemptRender(Object item);
}

The renderer internally checks for relevant types and return a string value if it can manage the object, null if not. Chaining would involve having a composite root renderer and going to next renderer in chain if previous one had returned null. So one renderer per "client layer", not per type.

The stack rendering code would then check for primitive types (as it does now), then call renderer.attemptRender(item), then if result is null fallback to printing the class name.

Example renderer implementation:

public MyErrorRenderer implements ErrorRenderer {
    public String attemptRender(Object item) {
        if (item == null) return null;
        if (item instanceof MyTypeWithShortToString) {
            //this one we know has a safe short toString()
            return item.toString();
        } else if (item instanceof MyOtherType) {
            //this one has a long toString but another safe short string representation method
            return ((MyOtherType) item).asShortString();
        }
        return null;
    }
}

what do you think?

[akarnokd]

I suggest you post a PR so we have a more complete picture how things can work.

[simonbasle]

sure, I think I may be able to find time tomorrow (CET) 👍

[simonbasle]

submitted a PR, had some problems during CI that seem unrelated to my work, any clue?

No such property: release.scope for class: org.gradle.api.internal.project.DefaultProject_Decorated

edit: nevermind the issue has disappeared

[benjchristensen]

Looks completed in #2632