| Summary: | Logging (via juli) does not reread configuration correctly when forced via LogManager indirect calls | ||
|---|---|---|---|
| Product: | Tomcat 6 | Reporter: | Mark Femal <mfemal> |
| Component: | Catalina | Assignee: | Tomcat Developers Mailing List <dev> |
| Status: | RESOLVED WONTFIX | ||
| Severity: | enhancement | ||
| Priority: | P2 | ||
| Version: | 6.0.29 | ||
| Target Milestone: | default | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: |
Full source code (changes done against pristine 6.0.29 src code)
cleaned up patch - sorry for not supplying the first time Sample Test code Sample test configuration - useful for test test.* package level changes Non-working TC7 patch (for reference) |
||
I am concerned that having viewed the JDK6 source code you then based the attached file on that. If that is the case, the attachment is not acceptable for use with in the Tomcat code base due to licensing issues. It the changes are permissible, not providing a patch isn't going to help the changes get applied. Created attachment 26036 [details]
cleaned up patch - sorry for not supplying the first time
Thanks for the extremely fast response. Supplied a unified diff. Regarding viewing LogManager.java, I made all changes on the Tomcat source file. Basically I moved a bunch of code out of addLogger to make it reusable (initial add and reload of config has to do many of the same things), ensured the properties aren't appended (or same properties just overwritten) on read calls (otherwise things that should drop out of logging.properties will not). This does account for statically defined loggers as the references will still exist, the level is just reset. None of the specific code is in the parent class - it is all tomcat specific. I'm not sure if viewing can be an issue of the class which this extends, but if so then hopefully this can be corrected by someone else after prioritizing. I'm no expert, but I don't believe I'm stepping on any toes on this as I made all changes based on changing the ClassLoaderLogManager.java file directly... OK, that sounds fine. The reference to the JDK6 source worried me but moving and cleaning up existing Tomcat code shouldn't be a problem. I haven't looked at the patch in detail yet but a quick look shows there are a few tabs in there. No need to provide a new patch. This is easy to fix but for future reference tabs should be replaced with 4 spaces. Created attachment 26037 [details]
Sample Test code
Created attachment 26038 [details]
Sample test configuration - useful for test test.* package level changes
The patch as provided does not work. It fails to take account of the caching of resources that occurs in the web application class loaders. I'm also curious how the re-reading of the CATALINA_BASE/conf/logging.properties was being triggered. Having reviewed the patch in more detail there are also a few places where unnecessary cosmetic changes are made. This makes the patch harder to review. I'm marking this as an enhancement and would be happy to revisit this to review a patch that addresses this issues. The patch will be applied to 7.0.x before it is applied to 6.0.x so patches against that code base are preferred. Created attachment 26115 [details]
Non-working TC7 patch (for reference)
You may find the attached port for TC7 useful. Or not.
No update provided in light of comment #7. Given the time that has past, resolving this as WONTFIX. The Tomcat community remains open to considering an updating patch that addresses comment #7 (and is updated for Tomcat 9). |
Created attachment 26035 [details] Full source code (changes done against pristine 6.0.29 src code) It is possible to normally do a LogManager.getLogManager() (which returns the JULI specific implementation). However, when calls to re-read the configuration via readConfiguration() or readConfiguration(inputStream) are done the current implementation does not properly re-read the config as the standard JVM LogManager does. In other words, the desired capability is to update the logging.properties file at run-time without forcing any other types of reloads (application context) or server restarts. To address this, it is necessary to look at run-time to see which loggers are already instantiated and manipulate the level at run-time based on the new configuration. Any loggers which are already instantiated should also have their levels reset if they are not in the current configuration (fall back to the default global level). In addition, in the current implementation a reset is not consistently done. I pulled the JDK6 source code to contrast what the JULI ClassLoaderLogManager was doing to do a comparison. Attached is the file I used for testing which should address these issues. It should be fairly trivial to add this into Tomcat 7 as well as not much has changed in this class in that version. With this change, sites can modify the site-specific logging.properties (context) and automatically reload those changes at run-time in the logging system. The attachment is all the code necessary to implement it (I did not do a patch).