jdbc-observations / datasource-micrometer

Micrometer Observation API instrumentation and Spring Boot 3 Auto-Configuration for JDBC APIs
Apache License 2.0
50 stars 8 forks source link

NPE in DataSourceObservationListener #22

Closed AnatoliyYakimov closed 11 months ago

AnatoliyYakimov commented 11 months ago

Hi. We get NPE while executing query with Spring JdbcTemplate.

java.lang.NullPointerException: Cannot invoke "java.lang.Boolean.booleanValue()" because "hasNext" is null
    at net.ttddyy.observation.tracing.DataSourceObservationListener.handleResultSetNext(DataSourceObservationListener.java:380)
    at net.ttddyy.observation.tracing.DataSourceObservationListener.afterMethod(DataSourceObservationListener.java:229)
    at net.ttddyy.dsproxy.listener.CompositeMethodListener.afterMethod(CompositeMethodListener.java:25)
    at net.ttddyy.dsproxy.listener.MethodExecutionListenerUtils.invoke(MethodExecutionListenerUtils.java:53)
    at net.ttddyy.dsproxy.proxy.SimpleResultSetProxyLogic.invoke(SimpleResultSetProxyLogic.java:29)
    at net.ttddyy.dsproxy.proxy.jdk.ResultSetInvocationHandler.invoke(ResultSetInvocationHandler.java:29)
    at jdk.proxy2/jdk.proxy2.$Proxy124.next(Unknown Source)
    at org.springframework.jdbc.core.RowMapperResultSetExtractor.extractData(RowMapperResultSetExtractor.java:93)
    at org.springframework.jdbc.core.RowMapperResultSetExtractor.extractData(RowMapperResultSetExtractor.java:61)
    at org.springframework.jdbc.core.JdbcTemplate$1.doInPreparedStatement(JdbcTemplate.java:723)
    at org.springframework.jdbc.core.JdbcTemplate.execute(JdbcTemplate.java:651)
    at org.springframework.jdbc.core.JdbcTemplate.query(JdbcTemplate.java:713)

NPE is here: .DataSourceObservationListener.handleResultSetNext

if (hasNext) {
    resultSetAttributes.context.incrementCount();
}

Variable hasNext:

    private void handleResultSetNext(MethodExecutionContext executionContext) {

        Boolean hasNext = (Boolean) executionContext.getResult();
        ...

And MethodExecutionContext is building like this:

  MethodExecutionContext methodContext = MethodExecutionContext.Builder.create()
                .target(proxyTarget)
                .method(method)
                .methodArgs(args)
                .connectionInfo(connectionInfo)
                .proxyConfig(proxyConfig)
                .build();

        MethodExecutionListener methodExecutionListener = proxyConfig.getMethodListener();
        methodExecutionListener.beforeMethod(methodContext);

        // method and args may be replaced in MethodExecutionListener
        Method methodToInvoke = methodContext.getMethod();
        Object[] methodArgsToInvoke = methodContext.getMethodArgs();

        final Stopwatch stopwatch = proxyConfig.getStopwatchFactory().create().start();
        Object result = null;
        Throwable thrown = null;
        try {
            result = callback.execute(proxyTarget, methodToInvoke, methodArgsToInvoke);
        } catch (Throwable throwable) {
            thrown = throwable;
            throw throwable;
        } finally {
            final long elapsedTime = stopwatch.getElapsedTime();

            methodContext.setElapsedTime(elapsedTime);
            methodContext.setResult(result);
            methodContext.setThrown(thrown);

            methodExecutionListener.afterMethod(methodContext);
        }

I think that if

 result = callback.execute(proxyTarget, methodToInvoke, methodArgsToInvoke);

throws an error, then variable result is null and we getting this NPE.

Solution will be to modify condition in if statement to avoid Boolean unboxing which causing NPE:

if (hasNext == Boolean.TRUE) {
    resultSetAttributes.context.incrementCount();
}
ttddyy commented 11 months ago

@AnatoliyYakimov Thanks for the report. To confirm, with the current code, this seems to happen when the actual ResultSet#next() throws an exception. Is this the scenario in your case?

AnatoliyYakimov commented 11 months ago

I think that's the reason. In logs we didn't see exception from ResultSet#next() because NPE were thrown from finally clause. I will try to reproduce this situation.

AnatoliyYakimov commented 11 months ago

I've created pull request that fix this issue #23