Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AbstractJackson2HttpMessageConverter throws exception if log level is ERROR [SPR-15760] #20315

Closed
spring-issuemaster opened this issue Jul 11, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Jul 11, 2017

Marc Brünisholz opened SPR-15760 and commented

If the log level is set to ERROR, the expression on line 161 in AbstractJackson2HttpMessageConverter evaluates to true.

Code of interest:

if (!logger.isWarnEnabled()) {
     return this.objectMapper.canDeserialize(javaType);
}
AtomicReference<Throwable> causeRef = new AtomicReference<>();
if (this.objectMapper.canDeserialize(javaType, causeRef)) {
     return true;
}

canDeserialize without the causeRef parameter rethrows exceptions that occur, which is not expected. canDeserialize(javaType, causeRef) should be called in any case. The log level should not influence if exceptions are thrown or not. The log level only determines what is logged or not.

We had a case where the log level was set to WARN on integration systems but to ERROR on production systems. On the integration system, the exception was logged as a warning and did not break the application. On the production system, the exception was logged with a full stack trace and rethrown, which broke the application. If the exception is considered to be logged as a warning, it should not be rethrown.

What is the initial intention of lines 161 - 163?


Affects: 4.3.6

Reference URL: https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java#L161

Referenced from: commits 5f767a8, 121a3bf

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2017

Juergen Hoeller commented

This code path dates back to older Jackson versions where there was no variant with a cause reference. At this point it remains a very minor optimization, namely not to have to provide an AtomicReference instance there when we're not going to log the outcome in any case. We can safely remove this extra check and go with a single code path.

That said, which Jackson version are you running against there? We haven't seen this propagation of exceptions before, so it might be a recent effect.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2017

Marc Brünisholz commented

We are running against Jackson version 2.8.5, which rethrows the exception if AtomicReference is not set:

public boolean canDeserialize(JavaType type)
{
    return createDeserializationContext(null,
            getDeserializationConfig()).hasValueDeserializerFor(type, null);
}
public boolean hasValueDeserializerFor(JavaType type, AtomicReference<Throwable> cause) {
    try {
        return _cache.hasValueDeserializerFor(this, _factory, type);
    } catch (JsonMappingException e) {
        if (cause != null) {
            cause.set(e);
        }
    } catch (RuntimeException e) {
        if (cause == null) { // earlier behavior
            throw e;
        }
        cause.set(e);
    }
    return false;
}
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2017

Juergen Hoeller commented

We are primarly concerned with JsonMappingException there which is not being rethrown... so I suppose your scenario involves some other RuntimeException? In any case, this is worth addressing on our end, so we're always going with the AtomicReference cause mechanism now.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 13, 2017

Marc Brünisholz commented

Yes, our scenario involves the RuntimeException BeanInstantiationException. However, if catched, the exception does not influence the functionality of our application in any way, so we are going to ignore it for the time being.

org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.hateoas.hal.Jackson2HalModule$HalLinkListDeserializer]: Constructor threw exception; nested exception is java.lang.NoSuchMethodError: com.fasterxml.jackson.databind.deser.std.ContainerDeserializerBase: method <init>(Ljava/lang/Class;)V not found
	at org.springframework.beans.BeanUtils.instantiateClass(BeanUtils.java:154)
	at org.springframework.beans.BeanUtils.instantiateClass(BeanUtils.java:102)
	at org.springframework.hateoas.hal.Jackson2HalModule$HalHandlerInstantiator.findInstance(Jackson2HalModule.java:682)
	at org.springframework.hateoas.hal.Jackson2HalModule$HalHandlerInstantiator.deserializerInstance(Jackson2HalModule.java:692)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.deserializerInstance(DefaultDeserializationContext.java:227)
	at com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.findDeserializerFromAnnotation(BasicDeserializerFactory.java:1777)
	
	...
	
	at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.executeProduceConsume(ExecuteProduceConsume.java:303)
	at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.produceConsume(ExecuteProduceConsume.java:148)
	at org.eclipse.jetty.util.thread.strategy.ExecuteProduceConsume.run(ExecuteProduceConsume.java:136)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:671)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$2.run(QueuedThreadPool.java:589)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NoSuchMethodError: com.fasterxml.jackson.databind.deser.std.ContainerDeserializerBase: method <init>(Ljava/lang/Class;)V not found
	at org.springframework.hateoas.hal.Jackson2HalModule$HalLinkListDeserializer.<init>(Jackson2HalModule.java:519)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
	at org.springframework.beans.BeanUtils.instantiateClass(BeanUtils.java:142)
	... 96 more	
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.