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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
