Replace servlet context logging with JUL logging #10138
Conversation
| @@ -0,0 +1,21 @@ | |||
| package com.google.gwt.user.server.rpc.logging; | |||
There was a problem hiding this comment.
Files are going to need copyright headers, and public classes will need at least some semblance of javadoc. Indentation for this codebase is two spaces, and imports have a super-picky sort order (I find the messages to be somewhat unclear, so looking for a similar class and using its imports as a template can help).
You probably want to run ant checkstyle locally before we run it on the PR - there would be a lot of logging for this.
| if (provider == null) { | ||
| synchronized (initLock) { | ||
| provider = loggerProvider.get(); | ||
| if (provider == null) { |
There was a problem hiding this comment.
This synchronization doesn't make any sense to me - it looks like you're doing a double-checked locking, but for a local variable? Why double check on an atomic at all? And after the CAS on 21, there is no other chance for the atomic to have held a null to begin with?
| ClassLoader[] loaders = new ClassLoader[] { | ||
| Thread.currentThread().getContextClassLoader(), | ||
| LogManager.class.getClassLoader(), | ||
| ClassLoader.getSystemClassLoader() | ||
| }; |
There was a problem hiding this comment.
The use of different classloaders seems to suggest that you expect that an application might have a class available to a different classloader than the one that the application is actively using (and its parents...), but then below you dedup in a way that doesn't respect which classloader the item came from?
What's the goal here? If you just want "classes the application can see", I think the default that ServiceLoader.load(Class<?>) uses is sufficient?
| providers = loadAllProviders(); | ||
| } | ||
| for (LoggerProvider provider : providers) { | ||
| if (provider.isDefault() || provider.getClass().getName().equals(name)) { |
There was a problem hiding this comment.
To confirm, isDefault is meant to sometimes override requesting the named provider, based on classloader order?
As written, if A is "default" but B is requested, if the ServiceLoader returns [A,B] you'll get A, but if it returns [B,A] you'll get B.
| String currentProviderName = currentProvider != null ? currentProvider.getClass().getName() : null; | ||
| if (!provider.getClass().getName().equals(currentProviderName)) { | ||
| loggerProvider.set(provider); | ||
| loggers.clear(); |
There was a problem hiding this comment.
This appears to clear out the known loggers, but any class that already holds a Logger instance will keep it (with its stale delegate).
| private LoggerDelegate createDelegate() { | ||
| LoggerProvider provider = LogManager.getLoggerProvider(); | ||
| LoggerDelegate newDelegate = provider.createLogger(name); | ||
| return delegate.compareAndSet(null, newDelegate) ? newDelegate : delegate.get(); |
There was a problem hiding this comment.
This is safe as written, but will cease to be safe if you ever traverse the LogManager.loggers.values() collection and try to replace/update the delegates. Not saying you want to or even should do that, but this could be made safe by using updateAndGet(old -> old == null ? newDelegate : old).
|
|
||
| @Override | ||
| public void error(String message, Throwable throwable) { | ||
| servletContext.log(message, throwable); |
There was a problem hiding this comment.
I think this is missing the error prefix? Could also delegate between the two impls (maybe even make LoggerDelegate.error(String) do that for you).
| servletContext.log(message, throwable); | |
| servletContext.log("ERROR: " + message, throwable); |
| try { | ||
| List<ClassNotFoundException> errs = new ArrayList<ClassNotFoundException>(); | ||
| SerializationPolicy policy = SerializationPolicyLoader.loadFromStream(in, errs); | ||
| logger.logInfo("Downloaded serialization policy from " + sourceName); |
There was a problem hiding this comment.
This was explicitly info before, but error now?
| && exceedsUncompressedContentLengthLimit(responseContent); | ||
| } | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
javadoc as to why it is deprecated, what the user should do instead
3 participants
Fixes #10136