Skip to content

SimpleAroundFilterObservation.wrap calls scope.close() duplicated #12787

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

Closed
kk-zu opened this issue Feb 27, 2023 · 1 comment
Closed

SimpleAroundFilterObservation.wrap calls scope.close() duplicated #12787

kk-zu opened this issue Feb 27, 2023 · 1 comment
Assignees
Labels
in: core An issue in spring-security-core type: bug A general bug
Milestone

Comments

@kk-zu
Copy link

kk-zu commented Feb 27, 2023

Describe the bug
spring-security:6.0.2
When SimpleAroundFilterObservation.wrap catched error, call error(ex); in catch block and stop(); in finally block.
Both methods call scope.close().

public Filter wrap(Filter filter) {
return (request, response, chain) -> {
start();
try {
filter.doFilter(request, response, chain);
}
catch (Throwable ex) {
error(ex);
throw ex;
}
finally {
stop();
}
};
}

First TracingContext.scopes became empty. Second, TracingContext.getScope return null. So NullPointerException is caused.

Show the flow after scope.close() is called.

  1. scope.close() is called in catch block.
  2. io.micrometer.observation.SimpleObservation.SimpleScope.close() is called.
  3. io.micrometer.observation.SimpleObservation.notifyOnScopeClosed() is called.
  4. io.micrometer.tracing.handler.TracingObservationHandler.onScopeClosed() is called.
  5. tracingContext.getScope().close(); is called.
  6. After it, this block is called.

    tracingContext.setSpanAndScope(span, () -> {
    scope.close();
    tracingContext.setScope(previousScopeOnThisObservation);
    });

  7. tracingContext.setScope(previousScopeOnThisObservation); set TracingContext.scopes empty because previousScopeOnThisObservation is null.
  8. When scope.close() is called for the second time in finally block, step 5 cause NullPointerException because tracingContext.getScope() return null.

To Reproduce
spring-security:6.0.2
micrometer-tracing:1.0.2
spring-boot-actuater-autoconfigure:3.0.3

Expected behavior
I think scope.close(); should be called only once.
Or, should I fix micrometer-tracing?

Thank you for your support.

@kk-zu kk-zu added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Feb 27, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Feb 27, 2023

Thanks, @kk-zu. Yes, I agree that close should only be called once. I will schedule this for the next maintenance release to get that corrected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: bug A general bug
Projects
Status: Done
Development

No branches or pull requests

3 participants