gzshilu commented on issue #2827: Improve ContextManager class of sniffer module URL: https://github.com/apache/skywalking/pull/2827#issuecomment-499330998 >coveralls : Many scenarios may cause the issue. I’m not clear about your situation, The situation that the memory leak is that the trace isn't finished, and the `StackDepth` don’t remove from thread local. And the issue maybe caused by two thread had to share the StackDepth variable. 1. The code of I added in this PR is after the traceContext be finished. 2. `StackDepth` in springmvc plugin had store in thread local, and also you later discovered it. 3. you do not need to worry share thread in threadLocal variable. > wu-sheng: RunningContext only closes when TracingContext close. Can't do like this. this is original code of `skywalking/master`: ``` java //ContextManager.stopSpan() : public static void stopSpan(AbstractSpan span) { if (get().stopSpan(span)) { CONTEXT.remove(); } } // TraceContext().stopSpan(span) : public boolean stopSpan(AbstractSpan span) { AbstractSpan lastSpan = peek(); if (lastSpan == span) { if (lastSpan instanceof AbstractTracingSpan) { AbstractTracingSpan toFinishSpan = (AbstractTracingSpan)lastSpan; if (toFinishSpan.finish(segment)) { pop(); } } else { pop(); } } else { throw new IllegalStateException("Stopping the unexpected span = " + span); } if (checkFinishConditions()) { finish(); } return activeSpanStack.isEmpty(); } // checkFinishConditions(): private boolean checkFinishConditions() { if (isRunningInAsyncMode) { asyncFinishLock.lock(); } try { if (activeSpanStack.isEmpty() && asyncSpanCounter.get() == 0) { return true; } } finally { if (isRunningInAsyncMode) { asyncFinishLock.unlock(); } } return false; } ``` **Read this original sourceCode and you will find that CONTEXT.remove() is executed after TraceContext finished，**if RUNTIME_CONTEXT.remove() after CONTEXT.remove(),I don't think there has any problem, and add this line is required. I searched globally for used by RUNTIME_CONTEXT in project, it never be removed**.It has a risk of out of memory.** Theadlocal is best to execute remove() after use completed in current Thread. I believe that if there is no RUNTIME_CONTEXT be added in this place, there will be other business bug , but we have not found it now. by my changes, I tested a day on my team’s project yesterday，did not find any questions for this change, and it solved the spring plugin's bug of this PR point(we don't use tomcat、undertow or jetty-server plugins,because there is an imperfection about display of operation-name of restful-api 。). I didn't want to reply because this PR was closed. I reply it because I think the technology should be stricter. Thanks for listening. @wu-sheng @ascrutae
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
