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

SLF4J MDC context clear() does not clear context #204

Open
Marcono1234 opened this issue Feb 10, 2021 · 2 comments
Open

SLF4J MDC context clear() does not clear context #204

Marcono1234 opened this issue Feb 10, 2021 · 2 comments

Comments

@Marcono1234
Copy link
Contributor

Marcono1234 commented Feb 10, 2021

Expected Behavior

As per documentation of Clearable, it should clear the context of the current thread:

Clearing the context removes not only an active context from the current thread, but also the potential stack of any parents.

The correct behavior would therefore probably be to clear the MDC data using MDC.clear().

Actual Behavior

If the context was obtained from MdcManager.getActiveContext() calling clear() does nothing.
If the context was created by initializing the MDC data in the current thread (initializeNewContext(...)) calling clear() only reverts to the previous state.

The way in which nl.talsmasoftware.context.ContextManagers.clearActiveContexts() currently behaves does therefore nothing because only MdcManager.MdcContext implements Clearable and an instance of it is obtained using getActiveContext().

Specifications

  • Version: 1.0.9-SNAPSHOT (d4a6283)
@Marcono1234 Marcono1234 changed the title SLF4J MDC context clear() might not clear context SLF4J MDC context clear() does not clear context Feb 10, 2021
@sjoerdtalsma
Copy link
Contributor

Thank you for pointing this out. I feel some reluctance of 'clearing' values that were not part of this context, so maybe reverting to the context as it was before is better than calling MDC.clear?

I guess the question is, are pre-existing MDC values part of the captured context or not? I like to leave things the way they were. Perhaps Clearable was a mistake to introduce?

Could you provide a unit-test that points out the bug so we can discuss this with a concrete example?

@Marcono1234
Copy link
Contributor Author

Perhaps Clearable was a mistake to introduce?

The documentation of ContextManagers.clearActiveContexts() provides a good example use-case where Clearable can be useful:

Appropriate use includes thread management, where threads are reused by some pooling mechanism. For example, it is considered safe to clear the context when obtaining a 'fresh' thread from a thread pool (as no context expectations should exist at that point).

When you are dealing with an executor or task execution facility provided by a third party library, there might be cases where you cannot be sure that previous tasks (not submitted by yourself) are properly cleaning up the context after themselves. In such cases calling ContextManagers.clearActiveContexts() can be useful to make sure your work task is run in a clean environment.
But as pointed out by #202, if the execution environment is controlled by yourself, then there is no need to call that method.


Could you provide a unit-test that points out the bug so we can discuss this with a concrete example?

MdcManagerTest actually has a test method which demonstrates this:

MdcManager mgr = new MdcManager();
MDC.put("dummy", "value");
// Test no-op for MdcManager
ContextManagers.clearActiveContexts();
assertThat(MDC.get("dummy"), is("value"));

I would expect that after calling clearActiveContexts() the MDC has been cleared (regardless of whether SLF4J or this library is managing it).

Though to clarify, I am fine with MdcManager.getActiveContext().close() having no effect since the rationale that the active context is managed by SLF4J makes sense here (but not for clear()).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants