Summary: | Improve thread trace logging in WebappClassLoader.clearReferencesThreads() | ||
---|---|---|---|
Product: | Tomcat 8 | Reporter: | Konstantin Kolinko <knst.kolinko> |
Component: | Catalina | Assignee: | Tomcat Developers Mailing List <dev> |
Status: | NEW --- | ||
Severity: | enhancement | CC: | ahmedhosni111 |
Priority: | P1 | ||
Version: | 8.0.5 | ||
Target Milestone: | ---- | ||
Hardware: | PC | ||
OS: | All | ||
Attachments: | Diff file fort to list all stack traces before clearing threads |
Description
Konstantin Kolinko
2014-05-19 21:58:14 UTC
Possible ways to improve this:
A. Reduce the log level for stack trace message from log.error() to log.info().
In the current code there are two "log.error()" messages at once, which looks like overkill.
> log.error(sm.getString("webappClassLoader.warnThread",
> getContextName(), threadName));
> log.error(sm.getString("webappClassLoader.stackTrace",
> threadName, getStackTrace(thread)));
B. Make this an opt-in feature. A new property on <Context>?
Or two properties, to distinct handling of "request threads" vs. "non-request" threads.
C. Merge similar logs into a single message?
(E.g. if there are many request threads that are stuck in the same place).
I think "A." is a good and easy way to go.
Any additional ideas, concerns?
I applied "A." in r1596090. D. Running org.apache.catalina.loader.TestWebappClassLoaderExecutorMemoryLeak, I think it will be more readable, if all the stack traces are printed at once, as 20-May-2014 hh:mm:ss.sss INFO [threadname] org.apache.catalina.loader.WebappClassLoader.clearReferencesThreads Stack traces of threads for web application [/foo]: Thread "name": stacktrace.... Thread "name": stacktrace.... Threads "name": stacktrace.... org.apache.catalina.connector.TestMaxConnections no longer generates this message org.apache.tomcat.websocket.TestWsWebSocketContainer no longer generates this message. Is there anything more to do here? If not, I'll resolve this as fixed. (In reply to Mark Thomas from comment #5) I am OK to treat this as enhancement, though Remy raised this as a serious issue. In any case it is not a stopper for tagging 8.0.next. On my TODO is to pursue idea from Comment 2 (print all traces at once). I do no have time today though. Implementing that needs some refactoring. My idea is to split threads loop into two loops, where the first one populates some informational structure and the second performs stopping (if enabled). I think the current code has a minor issue: It attempts to shut down executor for each encountered thread. Thus I think it does it N times if there are N running threads for the same Executor. This feature is off by default, though. It is not actually a serious issue, it is cosmetic but fairly irriating ... So enhancement is fine, but I am bumping up the priority. Created attachment 32241 [details] Diff file fort to list all stack traces before clearing threads Fix bug 56546 list all stack traces before clearing threads, using two loops one for each purposem as Konstantin Kolinko suggested |