Fix 1799 and Spring 33450 #2062
Conversation
…- Spring shutdown fails due to IllegalStateException
| try { | ||
| return source.getProperty(contextName, key); | ||
| } catch (Throwable ex) { | ||
| return null; |
There was a problem hiding this comment.
Shouldn't we better dump ex using the StatusLogger or something else?
There was a problem hiding this comment.
I don't think so. The issue that was reported simply causes error messages to be logged. The application is shutting down so an error during shutdown is irritating. Changing that to a warning doesn't really help much.
| public static AuthorizationProvider authorizationProvider(final PropertiesUtil props) { | ||
| return AuthorizationProvider.getAuthorizationProvider((PropertyEnvironment) props); | ||
| } |
There was a problem hiding this comment.
I am not really sure if ConfigurationFactory is the right place to provide the AuthorizationProvider. Does it really need to be placed here? I mean, the call site can themselves issue AuthorizationProvider.getAuthorizationProvider() too, ain't so?
There was a problem hiding this comment.
I agree that the method should not be duplicated.
Also, in order to make this PR back-portable to 2.x (it is a requirement, isn't it?), I would use LoggerContext as parameter.
Unless I am mistaken, all HTTP/HTTPS accesses are performed after a LoggerContext is created and the context is passed to the ConfigurationFactory as a parameter (since 2.7). This change would allow 3.x to configure separate credentials for separate web applications.
There was a problem hiding this comment.
The authorization provider is always created based on properties, partly because it can be used to authenticate with a remote source that provides your logging config.
There was a problem hiding this comment.
@ppkarwasz Correct. It is NOT a new method. Matt removed it a few weeks before I made this fix.
| startContext(ctx, classLoader); | ||
| } | ||
| } catch (IllegalStateException ex) { | ||
| return internalContext; |
There was a problem hiding this comment.
@rgoers, there are several points introduced in this PR that swallow exceptions. I would appreciate it if you can reconsider "Don't we need to communicate anything to the user at this point?" and if not, add some comments explaining why it is safe to swallow.
There was a problem hiding this comment.
I agree on the need to log a WARN message or even an ERROR message.
In these kind of situations users are really clueless on why their Log4j configuration is not working. Problems concerning multiple LoggerContexts are the hardest to debug. Users usually don't know that multiple logger contexts are possible (this is almost exclusive to Log4j Core).
There was a problem hiding this comment.
Previously the error was propagated back to LogManager. It would log a Warn message and then promptly get a ClassCastException as it returned the LoggerContext from logj-api, not a LoggerContext compatible with log4j-core. I think I might have a better way to deal with this so I will look into it.
| import org.apache.logging.log4j.spi.ExtendedLogger; | ||
|
|
||
| /** | ||
| * Creates a SimpleLoggerContext compatible with log4j-core. This class is internal to Log4j. |
There was a problem hiding this comment.
Would you mind explaining why do we need this? Where does SLC fall short? etc.
There was a problem hiding this comment.
org.apache.logging.log4j.simple.SimpleLoggerContext cannot be cast to org.apache.logging.log4j.core.LoggerContext, which is what the caller expects.
There was a problem hiding this comment.
[For future reference, the caller is this code in Spring Boot
private LoggerContext getLoggerContext() {
return (LoggerContext) LogManager.getContext(false);
}]
@rgoers: looking at this again, I don't think it is a good idea to add a new public method to LoggerContextFactory and an entire dummy class, just because Spring Boot did an unchecked cast.
Do you mind if I remove this class and fix the Spring Boot code instead?
| startContext(ctx, classLoader); | ||
| } | ||
| } catch (IllegalStateException ex) { | ||
| return internalContext; |
There was a problem hiding this comment.
I agree on the need to log a WARN message or even an ERROR message.
In these kind of situations users are really clueless on why their Log4j configuration is not working. Problems concerning multiple LoggerContexts are the hardest to debug. Users usually don't know that multiple logger contexts are possible (this is almost exclusive to Log4j Core).
| public static AuthorizationProvider authorizationProvider(final PropertiesUtil props) { | ||
| return AuthorizationProvider.getAuthorizationProvider((PropertyEnvironment) props); | ||
| } |
There was a problem hiding this comment.
I agree that the method should not be duplicated.
Also, in order to make this PR back-portable to 2.x (it is a requirement, isn't it?), I would use LoggerContext as parameter.
Unless I am mistaken, all HTTP/HTTPS accesses are performed after a LoggerContext is created and the context is passed to the ConfigurationFactory as a parameter (since 2.7). This change would allow 3.x to configure separate credentials for separate web applications.
This commit includes fixes for: - Spring 33450 - Spring shutdown fails due to IllegalStateException (#2062) - [LOG4J2-3618] Fix property source comparator and review suggestions from #2454.
Fix #1799
When errors are encountered creating a LoggerContext Log4j will return a SimpleLoggerContext. This usually fails as it doesn't extend LoggerContext in log4j-core. This change creates an InternalLoggerContext that is a wrapper to SimpleLoggerContext to avaoid this problem.
spring-projects/spring-boot#33450
During Shutdown a call to PropertiesUtil is made which calls the SpringPropertySource. However the Spring environment has already shutdown so an exception is thrown. This change handles the exception and ignores the ProeprtySource.