Matt, Thank you. If all is well, I’ll make the equivalent PR for the master branch.
Tim Tim Perry (916) 505-3634 > On Jan 28, 2021, at 6:49 PM, Matt Sicker <[email protected]> wrote: > > Great, thanks for the PR! I'll make sure to review this over the weekend. > >> On Thu, 28 Jan 2021 at 13:47, Tim Perry <[email protected]> wrote: >> >> I submitted pull request 463 for this work. >> https://github.com/apache/logging-log4j2/pull/463 >> >> Please let me know if there are any issues you would like me to address. >> >> Thank you, >> Tim >> >> >>> On Tue, Jan 26, 2021 at 3:27 PM Tim Perry <[email protected]> wrote: >>> >>> Okay, I've fully updated the code to use JUnit 5. >>> >>> I'm thinking of naming the ServletContextListener that should be >>> registered >>> as a listener in web.xml "Log4jShutdownOnContextDestroyedListener". In >>> my initial code it was named "Log4jServletDestroyedListenerTest" which >>> isn't very useful. The general pattern in this log4j-web artifact is to >>> name >>> things after the servlet interface they implement. However, now we have >>> two >>> classes implementing ServletContextListener so this class needs a name >>> that communicates its meaning better. Does the name >>> "Log4jShutdownOnContextDestroyedListener" make sense? It is the class >>> to register to do the log4j shutdown when the listener's >>> contextDestroyed method is called. >>> >>> I'm open to other suggestions. If no one has a strong preference I'll check >>> in the code with "Log4jShutdownOnContextDestroyedListener" as the name >>> and create the pull request tomorrow morning PST. >>> >>> I'm working on a branch based off release-2.x here: >>> >>> https://github.com/perry2of5/logging-log4j2/tree/release-2.x-configurableShutdown >>> >>> Thanks, >>> Tim >>> >>> >>>> On Tue, Jan 26, 2021 at 12:07 PM Matt Sicker <[email protected]> wrote: >>> >>>> The release-2.x branch is the current stable branch, while master is >>>> the 3.x branch right now. I'd suggest release-2.x since we're still >>>> likely to make at least one (if not more) more 2.x release before 3.0, >>>> though if you make a PR to both branches, that helps :) >>>> >>>> On Tue, 26 Jan 2021 at 13:47, Tim Perry <[email protected]> wrote: >>>>> >>>>> Yes, junit pulls in an old version of hamcrest. There are some nice >>>>> goodies in the newer hamcrest, but I just stuck with the existing >>>>> dependencies. I did upgrade everything to JUnit 5. >>>>> >>>>> What is the best branch to base my pull request on? I'm going to >>>>> re-apply the changes to get a clean history: one commit for the >>>>> testing upgrades and a second commit for substantive changes. >>>>> >>>>> Thanks, >>>>> Tim >>>>> >>>>> On Mon, Jan 25, 2021 at 1:15 PM Matt Sicker <[email protected]> wrote: >>>>> >>>>>> I believe that's already included. There's also AssertJ which might be >>>>>> a dependency. No strong preferences from me, though; just try to use >>>>>> the JUnit 5 API preferably as not everything has been migrated yet! :) >>>>>> >>>>>> On Mon, 25 Jan 2021 at 15:06, Tim Perry <[email protected]> wrote: >>>>>>> >>>>>>> Is it okay to import hamcrest to use in the tests? >>>>>>> >>>>>>> <dependency> >>>>>>> <groupId>org.hamcrest</groupId> >>>>>>> <artifactId>hamcrest</artifactId> >>>>>>> <version>2.2</version> >>>>>>> <scope>test</scope> >>>>>>> </dependency> >>>>>>> >>>>>>> >>>>>>> On Mon, Jan 25, 2021 at 10:44 AM Tim Perry <[email protected]> >>>> wrote: >>>>>>> >>>>>>>> Thank you, Matt. >>>>>>>> >>>>>>>> I'll get back to you after I've written some unit tests. >>>>>>>> >>>>>>>> Tim >>>>>>>> >>>>>>>> On Mon, Jan 25, 2021 at 10:37 AM Matt Sicker <[email protected]> >>>> wrote: >>>>>>>> >>>>>>>>> Go to https://github.com/apache/logging-log4j2/compare and >>>> click the >>>>>>>>> "compare across forks" link at the top to make a PR from your >>>> forked >>>>>>>>> repo. >>>>>>>>> >>>>>>>>> On Mon, 25 Jan 2021 at 12:15, Tim Perry <[email protected]> >>>> wrote: >>>>>>>>>> >>>>>>>>>> Matt, >>>>>>>>>> >>>>>>>>>> Thanks for clarifying. >>>>>>>>>> >>>>>>>>>> I'd be happy to write some tests and submit a PR. How should I >>>>>> submit a >>>>>>>>>> pull request? I don't think I can do it from the github repo I >>>>>> linked >>>>>>>>> to. >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Tim >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Mon, Jan 25, 2021 at 9:59 AM Matt Sicker <[email protected]> >>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> I'm just saying that I don't think any of the developers here >>>>>> would >>>>>>>>>>> object to functional changes you'd like to introduce here, >>>>>> especially >>>>>>>>>>> if you think this change makes sense for users other than >>>>>> yourself. >>>>>>>>>>> >>>>>>>>>>> If you submit your changes as a PR (and preferably add >>>> automated >>>>>> tests >>>>>>>>>>> if possible), we'd be happy to merge! >>>>>>>>>>> >>>>>>>>>>> On Mon, 25 Jan 2021 at 11:51, Tim Perry <[email protected]> >>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Matt, et al., >>>>>>>>>>>> >>>>>>>>>>>> I agree the deployment patterns you mention are more >>>> common and >>>>>> I >>>>>>>>>>> wouldn't >>>>>>>>>>>> start a new project embedding log4j in each WAR. However, >>>> I'm >>>>>>>>> trying to >>>>>>>>>>>> upgrade some old spring apps and my hands are tied on the >>>>>> deployment >>>>>>>>>>>> pattern. >>>>>>>>>>>> >>>>>>>>>>>> As mentioned in my comment on LOG4J2- 2624, the changes I >>>>>> proposed >>>>>>>>> don't >>>>>>>>>>>> fundamentally change the lifecycle hooks for web modules >>>> and >>>>>> each >>>>>>>>> class >>>>>>>>>>>> loader will still have its own independent log4j config. >>>> The >>>>>>>>> changes just >>>>>>>>>>>> provide the ability to stop log4j a little later. To me, >>>> this >>>>>> is a >>>>>>>>> low >>>>>>>>>>> risk >>>>>>>>>>>> change since the default behaviour is unchanged. If my >>>> approach >>>>>> of >>>>>>>>>>> passing >>>>>>>>>>>> the Log4jWebLifeCycle around in the ServletContext is >>>>>> unacceptable, >>>>>>>>> I'm >>>>>>>>>>>> happy to revisit the code and come up with another >>>> solution. >>>>>> Here >>>>>>>>> are the >>>>>>>>>>>> proposed changes: >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>>>>> >>>> https://github.com/perry2of5/logging-log4j2/commit/56455af53920d69ff7a49a63c5bbf38773069e8d >>>>>>>>>>>> >>>>>>>>>>>> I'd really like to fix these bugs. If you are telling me >>>> there >>>>>> are >>>>>>>>> more >>>>>>>>>>>> important things for the log4j team to work on and that >>>> there >>>>>> is no >>>>>>>>>>>> interest from the log4j committers to make these changes, >>>> I can >>>>>>>>> accept >>>>>>>>>>>> that. However, I think these changes would be welcomed by >>>> some >>>>>> log4j >>>>>>>>>>> users >>>>>>>>>>>> and I hope one of the log4j committers will work with me on >>>>>> solving >>>>>>>>> these >>>>>>>>>>>> issues. >>>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> Tim >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Sun, Jan 24, 2021 at 6:29 PM Matt Sicker < >>>> [email protected]> >>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> I'm not sure how much any of the devs here use the >>>> log4j-web >>>>>>>>> module >>>>>>>>>>>>> anymore (seems more common to use fat jars or one app per >>>>>> servlet >>>>>>>>>>>>> container instance at least), so it's hard to say about >>>> any >>>>>>>>>>>>> idiosyncrasies. The main purpose of the lifecycle hooks >>>> for >>>>>> web >>>>>>>>>>>>> modules is to allow each class loader to have its own >>>>>> independent >>>>>>>>>>>>> log4j config, though I'm not sure how common that >>>> deployment >>>>>>>>> pattern >>>>>>>>>>>>> is anymore. There are alternative strategies such as >>>> hooking >>>>>> into >>>>>>>>> the >>>>>>>>>>>>> server code itself so that logging can shutdown with the >>>>>> server >>>>>>>>> rather >>>>>>>>>>>>> than the individual applications, but that's a different >>>> use >>>>>> case. >>>>>>>>>>>>> >>>>>>>>>>>>> As for design ideas, I think I had initially wanted to >>>>>> refactor >>>>>>>>> the >>>>>>>>>>>>> web context API to mimic how Spring Framework registers >>>>>> itself in >>>>>>>>> the >>>>>>>>>>>>> ServletContext, though I never got around to doing that, >>>> and >>>>>> now I >>>>>>>>>>>>> typically use JVM-global logging configurations instead, >>>> so I >>>>>>>>> never >>>>>>>>>>>>> revisited that. >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, 22 Jan 2021 at 11:53, Tim Perry < >>>> [email protected]> >>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hello, >>>>>>>>>>>>>> >>>>>>>>>>>>>> I'd like to help fix LOG4J2-2624 and LOG4J2-1606. How >>>> can I >>>>>>>>> help? >>>>>>>>>>>>>> >>>>>>>>>>>>>> To me, the challenge is to ensure log4j is initialized >>>> the >>>>>> very >>>>>>>>> first >>>>>>>>>>>>> time >>>>>>>>>>>>>> the ServletContext is provided to any object during >>>>>> application >>>>>>>>>>> loading >>>>>>>>>>>>> and >>>>>>>>>>>>>> startup and to stop log4j during the very last event or >>>>>>>>> execution >>>>>>>>>>> hook a >>>>>>>>>>>>>> servlet 3.0 container exposes. Right now using the >>>> servlet >>>>>> 3.0 >>>>>>>>>>>>>> auto-configuration stops log4j too soon in some cases >>>> and >>>>>> using >>>>>>>>> the >>>>>>>>>>>>> servlet >>>>>>>>>>>>>> 2.5 configuration starts log4j too late in some cases. >>>>>>>>>>>>>> >>>>>>>>>>>>>> FWIW, I have posted a proposed fix in >>>>>>>>>>>>>> https://issues.apache.org/jira/browse/LOG4J2-1606. >>>> I'm not >>>>>>>>> sure if >>>>>>>>>>> it is >>>>>>>>>>>>>> the correct way to go. For one thing, it puts the >>>>>>>>> Log4jWebLifeCycle >>>>>>>>>>>>>> initializer into the ServletContext so that another >>>> object >>>>>> can >>>>>>>>> grab >>>>>>>>>>> it >>>>>>>>>>>>> and >>>>>>>>>>>>>> use it during log4j shutdown. Somewhere in the log4j >>>> dev >>>>>>>>> archives I >>>>>>>>>>> saw a >>>>>>>>>>>>>> note about moving data out of the ServletContext so >>>> that it >>>>>>>>> can't be >>>>>>>>>>>>>> overwritten. I'm not sure if my solution would need to >>>> be >>>>>>>>> modified or >>>>>>>>>>>>>> abandoned in light of this. >>>>>>>>>>>>>> >>>>>>>>>>>>>> The code changes I posted are based on a custom >>>> log4j-web >>>>>>>>> artifact I >>>>>>>>>>>>>> created for a client. It works for them on their >>>> Tomcat 8.x >>>>>>>>> servers. >>>>>>>>>>>>>> However, I'm not sure if I'm relying on any >>>> idiosyncratic >>>>>>>>> behaviour >>>>>>>>>>> of >>>>>>>>>>>>>> Tomcat or if there are earlier or later servlet >>>> container >>>>>>>>> events / >>>>>>>>>>> hooks >>>>>>>>>>>>>> that can be used to trigger configuration to happen >>>> earlier >>>>>> on >>>>>>>>>>> startup or >>>>>>>>>>>>>> stop log4j later when an application is stopped. >>>>>>>>>>>>>> >>>>>>>>>>>>>> If I can be of any help fixing these issues, I'd like >>>> to >>>>>> help. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I've gotten a lot of good use out of log4j over the >>>> years. >>>>>>>>> Thank you >>>>>>>>>>> for >>>>>>>>>>>>>> maintaining it. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Tim >>>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>> >>>
