MDC is leaking out to other threads

[tom59]

The MDC is leaking out of the context to other threads.

I am using micronaut 1.3.4 with MdcInstrumenter.
The MDC set in one thread leaks in another thread.
See the failing test there: https://github.com/tom59/micronaut-mdc-issue/blob/master/src/test/java/mn/mdc/instrumenter/MDCTest.java

Expected Behaviour

The MDC should not leak into other requests.

Actual Behaviour

The MDC sets for one request ends up in another request.
It is going there: https://github.com/tom59/micronaut-mdc-issue/blob/master/src/main/java/mn/mdc/instrumenter/RequestIdFilter.java#L25

Environment Information

• Operating System: Mac
• Micronaut Version: 1.3.4
• JDK Version: 11

Example Application

• https://github.com/tom59/micronaut-mdc-issue

[KrzysztofKowalczyk]

@lgathy I think you might have a solution to this one?

[lgathy]

I have struggled with this as well, my first attempt was very similar but didn't work either. Actually the filter implementation in the test is incorrect, as there is no guarantee that the doFinally() block cleaning up the MDC will run on the same thread where we called MDC.put(...), regardless of doing that on the filter direclty or in a doOnSubscribe() hook. The later actually gets only called after the controller method itself. The only way I was able to properly cover all reactive scenarios was to wrap the original publisher to instrument all method invocations to all its subscribers with MDC.put(...) before and MDC.remove(...) after the method call.

You can find my solution ported here. It turns the test scenario green.

Please note, that in its current state this solution also has some drawbacks:

1. MDC instrumentation does not try to store and restore previous context. In this scenario I found this adequate, also way more efficient.
2. I ran into strange consequences when a thread pool is triggered to instantiate a new thread upon handling a request, because the outermost task of the newly spun-up thread also gets instrumented with the triggering request's context leaking everything including the ServerRequestContext to all followup tasks on that thread. I think this is a bug, it may even pose a security risk. I don't think that instrumentation is needed there, but I did not have yet the time to further investigate. This instrumenter is protected against such leaks however as it overwrites the values in the MDC before method invocations, so subscribers are guaranteed to see the correct value in MDC all the time.
3. The helper classes used may as well be misplaced and they are not documented atm.
4. There may also be some parts of the helper classes which are not used for this fix, I didn't check thoroughly.

I'd be happy to polish this solution further after review to get accepted as a PR.

[graemerocher]

@lgathy please go ahead and polish and send your PR, even it is not merged as is it may influence a possible final solution to this.