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