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 >> > > > >> > > > > >> > > > >> > > >> > > > >> >> > > > > >> > > >> >
