Replace servlet context logging with logging facade by Lactantius · Pull Request #10138 · gwtproject/gwt

Lactantius · 2025-06-27T20:38:30Z

…er.server.rpc

niloc132 · 2025-10-29T14:39:12Z

@@ -0,0 +1,21 @@
+package com.google.gwt.user.server.rpc.logging;


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.

niloc132 · 2025-10-29T14:47:41Z

+        if (provider == null) {
+            synchronized (initLock) {
+                provider = loggerProvider.get();
+                if (provider == null) {


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?

I changed it to allow setting up a provider only once, so the locking is much more straightforward now, and might even work as intended.

The latest version of your branch still has double checked locking here:

gwt/user/src/com/google/gwt/user/server/rpc/logging/RpcLogManager.java

Line 69 in 752da22

if (result == null) {

Thanks. Fixed.

niloc132 · 2025-10-29T14:52:20Z

+        ClassLoader[] loaders = new ClassLoader[] {
+                Thread.currentThread().getContextClassLoader(),
+                LogManager.class.getClassLoader(),
+                ClassLoader.getSystemClassLoader()
+        };


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?

Removed. I was trying to get it to work better with Tomcat, but reading the documentation again suggested this wouldn't really do anything, and it didn't.

niloc132 · 2025-10-29T14:54:49Z

+            providers = loadAllProviders();
+        }
+        for (LoggerProvider provider : providers) {
+            if (provider.isDefault() || provider.getClass().getName().equals(name)) {


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.

Fixed. It will now pick a default only after checking through all the providers for a name match.

niloc132 · 2025-10-29T14:56:18Z

+        String currentProviderName = currentProvider != null ? currentProvider.getClass().getName() : null;
+        if (!provider.getClass().getName().equals(currentProviderName)) {
+            loggerProvider.set(provider);
+            loggers.clear();


This appears to clear out the known loggers, but any class that already holds a Logger instance will keep it (with its stale delegate).

Removed. No longer trying to allow resetting the provider.

niloc132 · 2025-10-29T15:01:04Z

+    private LoggerDelegate createDelegate() {
+        LoggerProvider provider = LogManager.getLoggerProvider();
+        LoggerDelegate newDelegate = provider.createLogger(name);
+        return delegate.compareAndSet(null, newDelegate) ? newDelegate : delegate.get();


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

niloc132 · 2025-10-29T15:06:23Z

+
+        @Override
+        public void error(String message, Throwable throwable) {
+            servletContext.log(message, throwable);


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

Suggested change

servletContext.log(message, throwable);

servletContext.log("ERROR: " + message, throwable);

At least some implementations of ServletContext#log seem to add some sort of prefix or level automatically when you use the overload with the Throwable argument, but I will check.

I seem to have hallucinated that. Fixed.

niloc132 · 2025-10-29T15:06:59Z

    try {
      List<ClassNotFoundException> errs = new ArrayList<ClassNotFoundException>();
      SerializationPolicy policy = SerializationPolicyLoader.loadFromStream(in, errs);
-      logger.logInfo("Downloaded serialization policy from " + sourceName);


This was explicitly info before, but error now?

niloc132 · 2025-10-29T15:08:15Z

        && exceedsUncompressedContentLengthLimit(responseContent);
  }

+  @Deprecated


javadoc as to why it is deprecated, what the user should do instead

No longer deprecated.

I don't understand your reply - perhaps you changed your mind, and it is still deprecated, but now has a comment? Or did you intend to drop this?

…tialization of RpcLoggerProvider Various other improvements as suggested by niloc32

…ovider()

… ServletContext)

niloc132 · 2025-12-17T20:02:22Z

  }

-  SerializationPolicy loadPolicy(String url, Logger logger) {
+  SerializationPolicy loadPolicy(String url, ServletContext servletContext) {


My interpretation of this was that the original authors took care to not permanently tie these internals to servlets, but let any server impl support adapting calls to be handled by the rpc impl.

However, the only way one could achieve that would be to actually build non-servlet code in the com.google.gwt.user.server.rpc package to be able to use this package protected class, so I think it doesn't actually matter in any realistic case.

The only two ways I can think around this are to:

Pass in the logger as before, which would make it harder to see where a logging statement came from

Since servlet context logging does not have any system for multiple named loggers, ServletContextLogger could just be a singleton, the init method of RemoteServiceServlet could set the ServletContext on it, and it could fall back to no-op or console logging or something if the ServletContext has not yet been set. (Since there are currently no log statements that could happen before the init method, this would not be a behavior change.) Then the ServletContext would no longer have to be a parameter on the logging methods. Something like this:

diff --git a/user/src/com/google/gwt/user/server/rpc/RemoteServiceServlet.java b/user/src/com/google/gwt/user/server/rpc/RemoteServiceServlet.java index 5d80b35b2..d4c054209 100644 --- a/user/src/com/google/gwt/user/server/rpc/RemoteServiceServlet.java +++ b/user/src/com/google/gwt/user/server/rpc/RemoteServiceServlet.java @@ -191,6 +191,7 @@ public class RemoteServiceServlet extends AbstractRemoteServiceServlet @Override public void init(ServletConfig config) throws ServletException { super.init(config); + RpcLogManager.setServletContext(config.getServletContext()); codeServerPort = getCodeServerPort(); } diff --git a/user/src/com/google/gwt/user/server/rpc/logging/RpcLogManager.java b/user/src/com/google/gwt/user/server/rpc/logging/RpcLogManager.java index 6a11dae2f..9686243e3 100644 --- a/user/src/com/google/gwt/user/server/rpc/logging/RpcLogManager.java +++ b/user/src/com/google/gwt/user/server/rpc/logging/RpcLogManager.java @@ -15,6 +15,7 @@ */ package com.google.gwt.user.server.rpc.logging; +import javax.servlet.ServletContext; import java.util.ServiceLoader; import java.util.concurrent.ConcurrentHashMap; @@ -49,6 +50,12 @@ public class RpcLogManager { name -> new RpcLogger(name, getLoggerProvider())); } + public static void setServletContext(ServletContext servletContext) { + if (loggerProvider instanceof ServletContextLoggerProvider) { + ((ServletContextLoggerProvider) loggerProvider).setServletContext(servletContext); + } + } + /** * Retrieves the current instance of the {@link RpcLoggerProvider}. * <br /> diff --git a/user/src/com/google/gwt/user/server/rpc/logging/ServletContextLoggerProvider.java b/user/src/com/google/gwt/user/server/rpc/logging/ServletContextLoggerProvider.java index bd0f67e4d..20a615691 100644 --- a/user/src/com/google/gwt/user/server/rpc/logging/ServletContextLoggerProvider.java +++ b/user/src/com/google/gwt/user/server/rpc/logging/ServletContextLoggerProvider.java @@ -26,33 +26,58 @@ public class ServletContextLoggerProvider implements RpcLoggerProvider { public ServletContextLoggerProvider() { } + public void setServletContext(ServletContext servletContext) { + ServletContextLogger.INSTANCE.servletContext = servletContext; + } + @Override public RpcLoggerDelegate createLogger(String name) { - return new ServletContextLogger(); + return ServletContextLogger.INSTANCE; } private static final class ServletContextLogger implements RpcLoggerDelegate { - ServletContextLogger() { } + private ServletContext servletContext; + + private static final ServletContextLogger INSTANCE = new ServletContextLogger(); + + private ServletContextLogger() { } ... } }

niloc132 · 2025-12-17T20:04:39Z


  /**
-   * Creates an client with the given configuration,
+   * Creates a client with the given configuration.


Not your fault, but this is still junk. Not required to make less-bad to merge, but here's my quick stab at it.

Suggested change

* Creates a client with the given configuration.

* Creates a client to load serialization policies from a dev mode server, with the given timeouts.

niloc132 · 2025-12-17T20:08:05Z

+    try {
+      logMessage.invoke(logger, warnLevel, message);
+    } catch (Exception e) {
+      System.err.println("Failed to log message: " + message);


Suggested change

System.err.println("Failed to log message: " + message);

System.err.println("Failed to log warning: " + message);

niloc132 · 2025-12-17T20:08:20Z

+    try {
+      logMessage.invoke(logger, errorLevel, message);
+    } catch (Exception e) {
+      System.err.println("Failed to log message: " + message);


Suggested change

System.err.println("Failed to log message: " + message);

System.err.println("Failed to log error: " + message);

niloc132 · 2025-12-17T20:08:28Z

+    try {
+      logThrowable.invoke(logger, errorLevel, message, throwable);
+    } catch (Exception e) {
+      System.err.println("Failed to log message: " + message);


Suggested change

System.err.println("Failed to log message: " + message);

System.err.println("Failed to log error: " + message);

niloc132 · 2025-12-17T20:21:58Z

+
+/**
+ * Uses reflection to create loggers that may be unavailable in some environments.
+ * Compatible with platform logging and Log4j; incompatible with SLF4j.


File a bug for SLF4J? I'm not sure if there'll be much interest (and there are adapters for JULI/etc), but it at least will answer the question and have a place to discuss if someone wants to pick up the work.

I removed all the reflective stuff. I did not see a good way to make a base class that would actually fit the APIs of all the main players, there will at least be some config file alterations no matter what, and if you actually have access to the relevant classes, it takes about ninety seconds to copy-paste the JulLoggerProvider, convert it to a different logger, and drop it in your own application or shared library.

niloc132 · 2025-12-17T20:26:16Z

+  protected Object getLogLevel(Class<?> levelClass, String levelName) {
+    for (Object level : levelClass.getEnumConstants()) {
+      if (level.toString().equals(levelName)) {
+        return level;
+      }
+    }
+    return null;
+  }


Since we already insist this is an enum (rather than a class that has static/final fields), maybe just use Enum.valueOf(Class, String)? It won't matter at all for perf, but you could effectively inline it away that way. You'll need a slightly better cast, but arguably you need one anyway since we're asserting that the Class is an enum type.

niloc132 · 2025-12-17T20:33:39Z

+      logger.warning(provider.getClass().getName());
+      logger.warning(String.valueOf(provider.isAvailable()));
+      logger.warning(String.valueOf(provider.isDefault()));


Should these be warnings? Probably some debug/trace level ("config", "fine" etc), but with some other content to explain what they mean?

These were accidentally committed, and are now removed. Thanks for catching them.

niloc132 · 2025-12-17T20:50:08Z

+  private RpcLoggerDelegate getDelegate() {
+    RpcLoggerDelegate result = this.delegate.get();
+    if (result == null) {
+      RpcLoggerProvider provider = RpcLogManager.getLoggerProvider();
+      RpcLoggerDelegate newDelegate = provider.createLogger(name);
+      result = delegate.updateAndGet(old -> old == null ? newDelegate : old);
+    }
+    return result;
+  }


Why is this lazy rather than initializing it when constructed, and avoiding the atomics? It is still racy, though the new/old guard at least ensures we'll always write to the same logger, but the extra logger can still be created.

Used this way, the updateAndGet call is just

delegate.compareAndSet(null, newDelegate); return delegate.get()

except this impl won't potentially loop.

With that said, at least from reading the review we don't have initialization cycles, so we can just initialize once in the constructor, right? Make RpcLogger final and don't let its constructor be public, now we guarantee that only RpcLogManager can create it, and RpcLogManager will always do so after it is itself initialied - then it is always safe to either call RpcLogManager.getLoggerProvider(), or make it part of the contract that RpcLogManager itself passes in the delegate with the name?

I'm sorry if I'm being thick and missing some contention, but I don't think we gain very much here, and we lose a little due to complexity and atomic performance.

Sorry I’m just now getting back to this. The idea was that, since getLogger is called statically at the top of RemoteServiceServlet, it would be hard to guarantee that a manual call to initialize would happen first. On the other hand, if someone just adds a logging statement to a static block that runs early enough in the lifecycle, the lazy initialization will suddenly break for reasons that might take a long time for the user to uncover. I am just going to reduce the choice mechanisms to either setting a system property or writing a custom provider that returns true for isDefault(). Without a way of guaranteeing, in all environments, that a manual initialization in application code will actually run first, it will probably cause frustration. That is why I was originally trying to allow the provider to be reset, but I think you correctly pointed out that such a mechanism would probably not be worth it.

niloc132 · 2025-12-17T20:52:51Z

+    if (loggerProvider.get() == null) {
+      synchronized (initLock) {
+        if (loggerProvider.get() == null) {
+          loggerProvider.set(provider);
+        }
+      }
+    }


Why use atomics if we're going to block on the entire manager's lock anyway? Both writes are guarded by a lock, maybe this should just be a standard double-checked lock - or just initialize eagerly with the class?

Most of these are apparently unused - but appear to provide an escape hatch for the three documented ways to specify a logger. Should these public initialize() methods be a 4th+ option?

The atomic is removed.

…y initialize the delegate

…roperly and would not work in many environments

…ting it on the ServletContextLogger

niloc132

I largely like this as it stands, though I think it could stand to have a little more cleanup before we say go - empty default constructors where access modifier matches the type, some added javadoc for public APIs.

I'm going to play with it a bit today so that I am sure I don't have more to say about how it is set up.

niloc132 · 2026-04-22T15:46:59Z

        && exceedsUncompressedContentLengthLimit(responseContent);
  }

+  @Deprecated


I don't understand your reply - perhaps you changed your mind, and it is still deprecated, but now has a comment? Or did you intend to drop this?

niloc132 · 2026-04-22T16:00:23Z

+/**
+ * A very simplified interface for logging RPC events.
+ */
+public interface RpcLoggerDelegate {


What if we dropped the existing RpcLogger, and renamed this interface to RpcLogger? The single place where we call new RpcLogger(...) could just as easily be rewritten as getLoggerProvider().createLogger(name), and otherwise RpcLogger and RpcLoggerDelegate have the exact same public API (though one is a final class and one an interface).

niloc132 · 2026-04-22T16:49:35Z

+ */
+public class RpcLogManager {
+
+  public static final String PROVIDER_PARAMETER_KEY = "gwt.rpc.logging";


PROVIDER_PROPERTY_KEY or the like, if we want it public (as documentation for downstream users)? Add Javadoc?

niloc132 · 2026-04-22T16:54:01Z

+  private static RpcLoggerProvider getLoggerProvider() {
+    if (loggerProvider == null) {
+      synchronized (RpcLogManager.class) {
+        if (loggerProvider == null) {
+          RpcLoggerProvider fallback = new ServletContextLoggerProvider();
+          String providerName = System.getProperty(PROVIDER_PARAMETER_KEY);
+          loggerProvider = loadProvider(providerName, fallback);
+        }
+      }
+    }
+    return loggerProvider;
+  }


I think this and the volatile field are still overkill - there's nothing the user can do to influence creation of the logger provider between class initialization and getLoggerProvider() being called (that window is very very narrow as written - setServletContext and getLogger both directly call getLoggerProvider). We're relying only on the system property being set (or not) and the service providers being on the classpath - those are typically set as they will remain at startup.

Why not make loggerProvider a static final field, and initialize it just once? The loadProvider method can still be used as is, inlined as an initializer, or the system property can be made a local instead of a param (its a private method anyway, no one can pass a different value).

g-keiser added 2 commits June 27, 2025 11:58

Replace servlet context logging with JUL logging in com.google.gwt.us…

1972cb5

…er.server.rpc

Add RPC logging facade backed by service loader

7593341

niloc132 reviewed Oct 29, 2025

View reviewed changes

niloc132 marked this pull request as draft October 29, 2025 15:15

g-keiser added 3 commits December 10, 2025 14:07

Add ServletContext parameter to RpcLogger methods, allow only one ini…

752da22

…tialization of RpcLoggerProvider Various other improvements as suggested by niloc32

Remove unneeded double-checked locking from RpcLogManager#getLoggerPr…

8479994

…ovider()

Added missing prefix to ServletContextLogger#error(String, Throwable,…

a193b4b

… ServletContext)

Lactantius marked this pull request as ready for review December 11, 2025 20:29

Lactantius changed the title ~~Replace servlet context logging with JUL logging~~ Replace servlet context logging with logging facade Dec 12, 2025

niloc132 reviewed Dec 17, 2025

View reviewed changes

g-keiser added 9 commits February 11, 2026 17:09

Remove reflective logger implementations

ff5c4a4

Remove accidentally committed logging

4c02c1d

Add real delegate to RpcLogger constructor instead of trying to lazil…

aed9365

…y initialize the delegate

Remove manual logger initialization mechanisms

370a386

Remove redundant atomic for RpcLoggerProvider

6c56728

Fix link to maven guide

949f5ef

Remove RpcLoggerProvider services file, which is not being packaged p…

fc0680f

…roperly and would not work in many environments

Remove reference to services directory from maven build script

68a3b9d

Correct continuation indent

5dd9a4f

Lactantius marked this pull request as draft February 13, 2026 15:06

g-keiser added 5 commits February 13, 2026 16:52

Remove ServletContext parameter from RpcLogger methods by instead set…

72127bc

…ting it on the ServletContextLogger

Improve javadoc comment

0d09368

Update javadoc

2091f1c

Merge initialize and get provider methods in RpcLogManager

fd4e266

Revert changes to RemoteServiceServletTest

ece5acf

Lactantius marked this pull request as ready for review February 25, 2026 21:09

niloc132 reviewed Apr 22, 2026

View reviewed changes

		@@ -0,0 +1,21 @@
		package com.google.gwt.user.server.rpc.logging;

	servletContext.log(message, throwable);
	servletContext.log("ERROR: " + message, throwable);

	* Creates a client with the given configuration.
	* Creates a client to load serialization policies from a dev mode server, with the given timeouts.

	System.err.println("Failed to log message: " + message);
	System.err.println("Failed to log warning: " + message);

	System.err.println("Failed to log message: " + message);
	System.err.println("Failed to log error: " + message);

Conversation

Lactantius commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niloc132 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!