Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Allow clearing the current Scope. #313

Open
wants to merge 2 commits into
base: v0.32.0
Choose a base branch
from

Conversation

carlosalberto
Copy link
Collaborator

Decided to take on being able to clear the current Scope, if any - this is done to prevent leaking Scope objects that were left around unintentionally.

If merged, this could become part of the next iteration of opentracing/specification#122 ;)

@yurishkuro @adriancole @felixbarny @tylerbenson

@coveralls
Copy link

coveralls commented Oct 18, 2018

Coverage Status

Coverage increased (+1.04%) to 76.225% when pulling e9868ee on carlosalberto:scope_manager_clear into ac3c666 on opentracing:v0.32.0.

@carlosalberto carlosalberto added the breaking_change Introduces a breaking change label Oct 18, 2018
Copy link
Contributor

@sjoerdtalsma sjoerdtalsma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is something that is good to have.
When returning threads back to their pool, you want to be sure not to leave things 'dangling'.

This could also really help issues like opentracing-contrib/java-jaxrs#109 and opentracing-contrib/java-jaxrs#87

* This method can be called to discard any previously leaked {@link Scope} objects
* that were unintentionally left active.
*/
void clear();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any difference in expected behavior between this calling

activate(null)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some implementations may keep a stack of activated scopes. My understanding is that clear() would clear the whole stack and activate would push another element on the stack.

Not quite sure but clear() may have slightly different semantics depending on whether the ScopeManager maintains a stack of Scopes or only holds the currently active scope.

Copy link
Contributor

@sjoerdtalsma sjoerdtalsma Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 My context manager -by default- maintains a stack.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the OT, but I'm thinking about doing the same. What are your reasons for that? Is your context manager on GitHub?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to share (@sjoerdtalsma @felixbarny) about the benefits and tradeoffs with actively managing a stack vs a simple reference to the current which thus has reference to its' parent.

Copy link
Contributor

@sjoerdtalsma sjoerdtalsma Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you ask: My context manager is 'pluggable' and has no real opinion on stacking or not. The default context uses a stack so you can easily change e.g. the current Locale just in a small piece of code and have everything else the way it was. Current supported contexts include MDC, Spring security context, Locale, originating ServletRequest, and OpenTracing Span. You can plug additional types via a ServiceLoader SPI.
For many situations, having a Closeable context that can be nested is the 'easiest' for users (as it returns things to the way they were). For OpenTracing Spans however, I just delegate to the configured ScopeManager.
More info: https://github.com/talsma-ict/context-propagation
For questions, feel free to ping me at gitter, always curious what others think.

@tylerbenson Sorry, just re-read your question (sorry for the late reply)

a simple reference to the current which thus has reference to its' parent

--> This forms what I meant by stack, it virtually forms a 'stack' of contexts this way (opposed to keeping only the current value)

Copy link
Contributor

@sjoerdtalsma sjoerdtalsma Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tradeoffs for stacking:
➕ Support nesting
➕ User experience: 'return things they way they were' / 'works as expected' in many cases
➖ Obviously a bigger chance of bigger memory leaks if open-close semantics are not correctly used.
➖ More memory consumption / garbage collecting

Copy link
Contributor

@felixbarny felixbarny Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More memory consumption / garbage collecting

Not sure if that is true. The stack won't be collected unless the thread terminates.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea. It is a hack, but a similar hack can be achieved by allowing a closure that clears a scope. ex passing a null tracecontext to the thing that scopes the active span

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the benefit of passing null is you can still provide for "descoping that null" which is in easiest form returning a noop scope object when it was already null.

@felixbarny
Copy link
Contributor

Who is supposed that method in which circumstances?

@sjoerdtalsma
Copy link
Contributor

@felixbarny

Who is supposed that method in which circumstances?

I can imagine some servlet filter right before returning the thread to the pool. Or a custom job scheduler, -again- also before returning a worker thread to the pool. i.e. the one who controls threads, but may not control the correct activation / closing of current scopes of all called code.

@sjoerdtalsma
Copy link
Contributor

Long time ago in the times of application servers and class loaders, I even went as far to create a list of all static ThreadLocal uses in our entire codebase (including libraries) and remove() all of them after each web request.
It was a hack, but it made several memory leaks disappear and gave us time to perform a more thorough analysis of the root cause(s).

@carlosalberto
Copy link
Collaborator Author

So it seems we have some agreement on this - any opinion @objectiser @yurishkuro @CodingFabian ?

@CodingFabian
Copy link

I do not fully understand the purpose. Is this a band aid that can be used whenever something unexpected happens? A util for libraries to clear up the mess a user might have created or something that has legitimate use by a user?
users could just decide to not commit a span or create a new one.

While this api is technically no problem, I wonder what the guidelines for users are. How is this api intended to be used by end users.

@carlosalberto
Copy link
Collaborator Author

Hey @CodingFabian

So this is more of a preventive use case, I'd say, and is not expected to be used by final users most of the time (same as ScopeManager.active()).

Quoting @sjoerdtalsma:

I can imagine some servlet filter right before returning the thread to the pool

And the issue around opentracing-contrib/java-jaxrs#109 could use this one.

@carlosalberto
Copy link
Collaborator Author

Hey @CodingFabian Did the reason given above sounds right to you? Let me know ;)

@yurishkuro @opentracing/opentracing-java-maintainers Opinion on this?

sjoerdtalsma added a commit to talsma-ict/context-propagation that referenced this pull request May 6, 2019
Tests must be extended when the following PR is merged:
opentracing/opentracing-java#313

Signed-off-by: Sjoerd Talsma <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking_change Introduces a breaking change enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants