[
https://issues.apache.org/jira/browse/GROOVY-12386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113284#comment-18113284
]
ASF GitHub Bot commented on GROOVY-12386:
-----------------------------------------
Copilot commented on code in PR #2908:
URL: https://github.com/apache/groovy/pull/2908#discussion_r3967752455
##########
subprojects/groovy-json/src/main/java/org/apache/groovy/json/internal/Exceptions.java:
##########
@@ -31,7 +31,23 @@
*/
public class Exceptions {
- private static final System.Logger LOGGER =
System.getLogger(Exceptions.class.getName());
+ /**
+ * Holds the logger so that loading this class does not resolve
+ * {@code System.Logger}, a Java 9 API absent from some runtimes built on
+ * the JDK class library (Android's ART); where it is unavailable the
+ * exception prints its stack trace the ordinary way (GROOVY-12386).
+ */
+ private static final class Log {
+ static final System.Logger LOGGER = create();
+
+ private static System.Logger create() {
+ try {
+ return System.getLogger(Exceptions.class.getName());
+ } catch (Throwable t) {
+ return null;
+ }
+ }
+ }
Review Comment:
This change aims to avoid requiring the Java 9 logging API on runtimes like
Android, but the nested `Log` class still has hard bytecode references to
`System.Logger` (and likely `System.Logger.Level` via `ERROR`). On runtimes
where `System.Logger` is absent, loading `Exceptions$Log` will fail with a
linkage error before `create()` can run and before `catch (Throwable)` can
return `null`, so the fallback path in `printStackTrace()` won’t be reached. To
make the fallback effective, avoid direct type references by using reflection
(store the logger as `Object` and invoke `log` reflectively, including
resolving `java.lang.System$Logger$Level` by name), or isolate the
System.Logger-based implementation into a Java 9+ specific artifact (e.g.,
MR-JAR / multi-release split) and keep the base runtime free of `System.Logger`
symbols.
##########
subprojects/groovy-json/src/main/java/org/apache/groovy/json/internal/Exceptions.java:
##########
@@ -234,10 +250,14 @@ public void printStackTrace(PrintWriter s) {
*/
@Override
public void printStackTrace() {
- LOGGER.log(ERROR, this.getMessage());
+ if (Log.LOGGER == null) {
+ super.printStackTrace();
+ return;
+ }
+ Log.LOGGER.log(ERROR, this.getMessage());
if (getCause() != null) {
- LOGGER.log(ERROR, "This Exception was wrapped, the original
exception stack trace is:", getCause());
+ Log.LOGGER.log(ERROR, "This Exception was wrapped, the
original exception stack trace is:", getCause());
Review Comment:
This change aims to avoid requiring the Java 9 logging API on runtimes like
Android, but the nested `Log` class still has hard bytecode references to
`System.Logger` (and likely `System.Logger.Level` via `ERROR`). On runtimes
where `System.Logger` is absent, loading `Exceptions$Log` will fail with a
linkage error before `create()` can run and before `catch (Throwable)` can
return `null`, so the fallback path in `printStackTrace()` won’t be reached. To
make the fallback effective, avoid direct type references by using reflection
(store the logger as `Object` and invoke `log` reflectively, including
resolving `java.lang.System$Logger$Level` by name), or isolate the
System.Logger-based implementation into a Java 9+ specific artifact (e.g.,
MR-JAR / multi-release split) and keep the base runtime free of `System.Logger`
symbols.
##########
subprojects/groovy-json/src/main/java/org/apache/groovy/json/internal/Sys.java:
##########
@@ -29,7 +29,23 @@
*/
class Sys {
- private static final System.Logger LOGGER =
System.getLogger(Sys.class.getName());
+ /**
+ * Holds the logger so that loading this class does not resolve
+ * {@code System.Logger}, a Java 9 API absent from some runtimes built on
+ * the JDK class library (Android's ART); the logger is only needed on a
+ * failure path (GROOVY-12386).
+ */
+ private static final class Log {
+ static final System.Logger LOGGER = create();
+
+ private static System.Logger create() {
+ try {
+ return System.getLogger(Sys.class.getName());
+ } catch (Throwable t) {
+ return null;
+ }
+ }
+ }
Review Comment:
Catching `Throwable` here will also swallow fatal VM conditions (e.g.,
`OutOfMemoryError`, `StackOverflowError`) and turn them into a silent `null`
logger. If you keep the try/catch approach, narrow the catch to the expected
linkage failures (e.g., `LinkageError`/`NoSuchMethodError`) and optionally
`SecurityException`, or rethrow `VirtualMachineError` (and possibly
`ThreadDeath`) to avoid masking unrecoverable failures.
##########
subprojects/groovy-json/src/main/java/org/apache/groovy/json/internal/Sys.java:
##########
@@ -55,7 +71,9 @@ class Sys {
v = new BigDecimal("1.9");
}
} catch (Exception ex) {
- LOGGER.log(WARNING, "Unable to determine build number or
version", ex);
+ if (Log.LOGGER != null) {
+ Log.LOGGER.log(WARNING, "Unable to determine build number
or version", ex);
+ }
Review Comment:
Catching `Throwable` here will also swallow fatal VM conditions (e.g.,
`OutOfMemoryError`, `StackOverflowError`) and turn them into a silent `null`
logger. If you keep the try/catch approach, narrow the catch to the expected
linkage failures (e.g., `LinkageError`/`NoSuchMethodError`) and optionally
`SecurityException`, or rethrow `VirtualMachineError` (and possibly
`ThreadDeath`) to avoid masking unrecoverable failures.
##########
subprojects/groovy-json/src/test/groovy/org/apache/groovy/json/internal/LazyLoggerTest.groovy:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.groovy.json.internal
+
+import org.junit.jupiter.api.Test
+
+import static org.junit.jupiter.api.Assertions.assertNotNull
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * GROOVY-12386: the internal classes create their {@code System.Logger}
lazily,
+ * in a nested holder, so that loading them does not require the Java 9 logging
+ * API, which Android's runtime lacks.
+ */
+class LazyLoggerTest {
+
+ @Test
+ void loggersLiveInNestedHoldersNotInTheClassesThemselves() {
+ [Sys, Exceptions].each { owner ->
+ assertTrue(owner.declaredFields.every { it.type != System.Logger
}, owner.name)
+ def holder = owner.declaredClasses.find { it.simpleName == 'Log' }
+ assertNotNull(holder, owner.name)
+ def field = holder.getDeclaredField('LOGGER')
+ field.accessible = true
+ assertNotNull(field.get(null), 'the logger resolves on a JVM')
Review Comment:
Using `field.accessible = true` can fail under stronger access checks (e.g.,
`InaccessibleObjectException` in some Java 9+ module configurations). Consider
using `trySetAccessible()` (when available) and asserting it succeeded (or
skipping the assertion with an assumption) to avoid brittle test failures in
more restricted runtimes.
> groovy-json: keep System.Logger out of internal static initialisers
> -------------------------------------------------------------------
>
> Key: GROOVY-12386
> URL: https://issues.apache.org/jira/browse/GROOVY-12386
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Involves: two internal classes, Sys and Exceptions, create a System.Logger in
> a static field. The spike switched them to java.util.logging. A gentler
> option is a lazy holder so the logger is only created when something is
> actually logged, which is an error path in both classes.
> Impact on normal usage: with the JUL switch, users who installed a custom
> System.LoggerFinder would see these two rare messages routed differently.
> With the lazy option nothing changes on a JVM, so I would prefer that.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
