-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add special handler to allow logger messages during shutdown #1387
Conversation
Example without this patch:
Example WITH this patch:
|
Bummer about the shutdown hooks. Some questions:
|
@gianm : The dependency is exactly one java class https://github.com/DjDCH/Log4j-StaticShutdown/blob/master/src/main/java/com/djdch/log4j/StaticShutdownCallbackRegistry.java that we would pretty much have to copy to get the functionality. It is MIT license so direct copy is probably a no-go. The CLI classes pickup This will revert to the prior behavior if the property is missing. |
Looking at what that class does, it basically just extends Log4j's ShutdownCallbackRegistry. It's pretty difficult to do programmatic stuff to adjust the configuration of the log4j subsystem when you use properties files to initialize it. We essentially need to set the system property before the log4j system is initialized. I believe it is initialized by the first call to So, if we were to actually add a static initializer there to set the system property for us, we would likely be able to register our own shutdown handler instead of having it use the internal one. I would then recommend that we create our own ShutdownCallbackRegistry that
|
@cheddar : Doing such a thing in the Logger would also be a multi-system intrusion of log4j instead of slf4j abstraction. We cannot rely on the JVM shutdown hook because they are fired essentially randomly. We have to be able to specify that the logger shutdown hook runs after the Lifecycle shutdown, which is not the case if both are simply shutdown hooks (unless you know some magic you would like to share :) ) |
4333af4
to
9d7ba77
Compare
@cheddar : Modified |
} | ||
}; | ||
shutdownCallbacks.add(cancellable); | ||
if (!isStarted()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why this means that we are shutting down in the middle of registering. Couldn't it just not have been started in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method has a !isStarted check at the top. If it is started at the top but not started here, it is because of a shutdown in progress.
Is it at all possible to add a check somewhere that can notice if the static property setting stuff was too late and warn/error about it? |
"shutdownCallbackRegistry" | ||
); | ||
shutdownCallbackRegistryField.setAccessible(true); | ||
final Log4jShutdown log4jShutdown = (Log4jShutdown) shutdownCallbackRegistryField.get(LogManager.getFactory()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need reflection to reach in and call the shutdowner? Can't we just get it injected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log4j does its own instantiation based on the system property and does not inject through guice. This was the only way at the time I could find of grabbing the log4j instance. Seems like a bug in log4j2 imho (unless I'm missing something)
9d7ba77
to
d6dbdb7
Compare
@cheddar There is now a check in the configure step via class cast and a warning if the class cast fails. |
d6dbdb7
to
22a4703
Compare
👍 |
22a4703
to
a3dedfc
Compare
.to(Log4jShutterDowner.class) | ||
.asEagerSingleton(); | ||
} | ||
catch (ClassNotFoundException | ClassCastException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be catching NoClassDefFoundError too? I think that would pop up if the log4j stuff (like org.apache.logging.log4j.core.impl.Log4jContextFactory) isn't on the classpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added LinkageError
* Adds a special PropertyChecker interface which is ONLY for setting string properties at the very start of psvm
a3dedfc
to
7a2ceef
Compare
still 👍 |
Add special handler to allow logger messages during shutdown
This gets rid of those pesky
FATAL Unable to register shutdown hook because JVM is shutting down.
messages.Previously the shutdown hooks fire essentially at random. This was causing logging messages to regularly be lost during the shutdown cycle if the log4j shutdown hook was fired before some of the ones that use logging in their shutdown messages.
This patch (with the appropriate server setting) causes the logger shutdown to fire as the last lifecycle.stop() item.
Omitting the server property simply reverts to the prior behavior of regularly discarding logger messages during shutdown.