OnError while emitting onNext value: object.toString #1401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Forgot to include the results of the test: Here is the expected error: Heres what we get with an exception in toString And a system,err output of |
|
RxJava-pull-requests #1349 SUCCESS |
|
This rather defeats the point of What change do you recommend that doesn't defeat the current purpose of |
|
The exception can still contain the value, it just shouldn't be eagerly toStringed into the base exception message. If the user code wants to handle those exceptions and print out the related object they are free to do so. In any case It is only sometimes helpful to know what value is being processed when a request failed. What if the cause of the failure is a downed connection to an external service or a cache being disconnected, something unrelated to the data being processed. It doesn't matter what value caused the error, any value would have failed and seeing this in the log may cause people to assume it was related to this specific value. try{
// processing
}catch(Exception e){
logger.log("Exception in processing: ", e);
if(e.getCause() instanceOf OnNextValue){
logger.log("Value that was being processed when it failed: ", ((OnNextValue)e.getCause()).getValue);
}
} |
Obviously, but it's very valuable when it is indeed related to the value. How about we include the toString value if it is a primitive type ( |
|
I think that is a good idea, we can also print out the classname of unknown objects. How about this? /**
* Render the object if it is a basic type. This avoids the library making potentially expensive
* or calls to toString() which may throw exceptions. See PR #1401 for details.
*
* @param value
* the item that the Observable was trying to emit at the time of the exception
* @return a string version of the object if primitive, otherwise the classname of the object
*/
private static String renderValue(Object value){
if(value == null){
return "null";
}
if(value.getClass().isPrimitive()){
return value.toString();
}
if(value instanceof String){
return (String)value;
}
return value.getClass().getSimpleName() + ".class;
}I played with the wording a bit to try to make it obvious this is a class name of the instance, not the Class itself but everything I thought of might be misinterpreted. |
…bjects without calling toString (PR ReactiveX#1401)
|
RxJava-pull-requests #1352 SUCCESS |
|
I like it. Merging. Thanks @samhendley |
benjchristensen
added a commit
that referenced
this pull request
Jul 7, 2014
OnError while emitting onNext value: object.toString
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I know this is probably a helpful error message in some cases but this can be a really costly operation when an objects toString is an expensive call or contains alot of output. I don't think we should be printing this in any case but if so it should be on demand (overload of getMessage()) rather than eagerly.
In my case it is causing a toString of a large context object that is normally only used for debugging purposes which makes the exception logs hard to use and they are rolling over the log files very quickly.
There is an added danger that if there is a bug in the toString method it will cause inconsistent exception creation. If the object throws an exception while rendering a string it will actually end up not seeing the real exception.