garydgregory commented on code in PR #390:
URL: https://github.com/apache/commons-jexl/pull/390#discussion_r2508301168
##########
src/changes/changes.xml:
##########
@@ -30,6 +30,9 @@
<release version="3.6.0" date="2025-11-08"
description="This is a feature and maintenance release. Java
8 or later is required.">
<!-- FIX -->
+ <action dev="henrib" type="fix" issue="JEXL-448">
+ Cache handling is perfectible.
Review Comment:
"Cache handling is perfectible." is not helpful IMO. Would you please
describe what's changed?
##########
src/main/java/org/apache/commons/jexl3/parser/ASTIdentifierAccessJxlt.java:
##########
@@ -57,7 +57,7 @@ public void setIdentifier(final String src, final Scope
scope) {
if (src != null && !src.isEmpty()) {
final JexlEngine jexl = JexlEngine.getThreadEngine();
if (jexl != null) {
- final JxltEngine jxlt = jexl.createJxltEngine();
+ final JxltEngine jxlt = jexl.createJxltEngine(true, -1,
'$','#');
Review Comment:
I would add a comment explaining why '$' and '#' are special here.
##########
src/main/java/org/apache/commons/jexl3/internal/Scope.java:
##########
@@ -93,6 +94,17 @@ public Scope(final Scope scope, final String... parameters) {
vars = 0;
parent = scope;
}
+ /**
+ * Gets an unmodifiable view of a scope"e;s symbols map.
+ * @param scope the scope
+ * @return the symbols map
+ */
+ public static Map<String, Integer> getSymbolsMap(final Scope scope) {
Review Comment:
Shouldn't this be package-private?
##########
src/main/java/org/apache/commons/jexl3/parser/ASTJxltLiteral.java:
##########
@@ -70,7 +70,7 @@ void setLiteral(final String src, final Scope scope) {
if (src != null && !src.isEmpty()) {
final JexlEngine jexl = JexlEngine.getThreadEngine();
if (jexl != null) {
- final JxltEngine jxlt = jexl.createJxltEngine();
+ final JxltEngine jxlt = jexl.createJxltEngine(true, -1,
'$','#');
Review Comment:
I would add a comment explaining why '$' and '#' are special here.
##########
src/test/java/org/apache/commons/jexl3/internal/SourceCacheTest.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * 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
+ *
+ * https://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.commons.jexl3.internal;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.commons.jexl3.ConcurrentCache;
+import org.apache.commons.jexl3.JexlBuilder;
+import org.apache.commons.jexl3.JexlCache;
+import org.apache.commons.jexl3.JexlEngine;
+import org.apache.commons.jexl3.JexlFeatures;
+import org.apache.commons.jexl3.JexlScript;
+import org.junit.jupiter.api.Test;
+
+public class SourceCacheTest {
Review Comment:
This class uses an unusual 2 spaces for indentation instead of the 4 in
other classes. Best to normalize to 4 spaces IMO.
##########
src/main/java/org/apache/commons/jexl3/internal/MetaCache.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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
+ *
+ * https://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.commons.jexl3.internal;
+
+import java.lang.ref.Reference;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.IntFunction;
+
+import org.apache.commons.jexl3.JexlCache;
+
+/**
+ * A meta-cache that tracks multiple JexlCache instances via weak references.
+ * <p>
+ * Each JexlCache created by this MetaCache is held via a WeakReference,
+ * allowing it to be garbage collected as soon as no strong references exist.
+ * </p>
+ * <p>
+ * This allows for collective management of multiple caches, in particular
clearing all caches at once.
+ * This operation is typically called when the uberspect class loader needs to
change.
+ * </p>
+ */
+public class MetaCache {
+ // The factory to create new JexlCache instances
+ private final IntFunction<JexlCache<?, ?>> factory;
+ // The set of JexlCache references
+ private final Set<Reference<JexlCache<?,?>>> references;
+ // Queue to receive references whose referent has been garbage collected
+ private final ReferenceQueue<JexlCache<?,?>> queue;
+
+ public MetaCache(final IntFunction<JexlCache<?, ?>> factory) {
+ this.factory = factory;
+ this.references = new HashSet<>();
+ this.queue = new ReferenceQueue<>();
+ }
+
+ @SuppressWarnings("unchecked")
+ public <K,V> JexlCache<K,V> createCache(final int capacity) {
+ if (capacity > 0) {
+ JexlCache<K, V> cache = (JexlCache<K, V>) factory.apply(capacity);
+ if (cache != null) {
+ synchronized(references) {
+ // The reference is created with the queue for automatic cleanup
+ references.add(new WeakReference<>(cache, queue));
+ // Always clean up the queue after modification to keep the set tidy
+ cleanUp();
+ }
+ }
+ return cache;
+ }
+ return null;
+ }
+
+ public void clearCaches() {
+ synchronized (references) {
+ for (Reference<JexlCache<?,?>> ref : references) {
+ JexlCache<?,?> cache = ref.get();
+ if (cache != null) {
+ cache.clear();
+ }
+ }
+ cleanUp();
+ }
+ }
+
+ /**
+ * Cleans up all references whose referent (the cache) has been garbage
collected.
+ * <p>This method must be invoked while holding the lock on {@code
references}.</p>
+ *
+ * @return The remaining number of caches.
+ */
+ @SuppressWarnings("unchecked")
+ private int cleanUp() {
+ // The poll() method returns the next reference object in the queue, or
null if none is available.
+ Reference<JexlCache<?,?>> reference;
+ while ((reference = (Reference<JexlCache<?, ?>>) queue.poll()) != null) {
+ // Remove the reference from the set
+ references.remove(reference);
+ }
+ return references.size();
+ }
+
+ public int size() {
Review Comment:
Needs a Javadoc, especially since there is a side-effect.
##########
src/main/java/org/apache/commons/jexl3/internal/MetaCache.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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
+ *
+ * https://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.commons.jexl3.internal;
+
+import java.lang.ref.Reference;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.IntFunction;
+
+import org.apache.commons.jexl3.JexlCache;
+
+/**
+ * A meta-cache that tracks multiple JexlCache instances via weak references.
+ * <p>
+ * Each JexlCache created by this MetaCache is held via a WeakReference,
+ * allowing it to be garbage collected as soon as no strong references exist.
+ * </p>
+ * <p>
+ * This allows for collective management of multiple caches, in particular
clearing all caches at once.
+ * This operation is typically called when the uberspect class loader needs to
change.
+ * </p>
+ */
+public class MetaCache {
Review Comment:
This class uses an unusual 2 spaces for indentation instead of the 4 in
other classes. Best to normalize to 4 spaces IMO.
Can this class be package-private?
##########
src/test/java/org/apache/commons/jexl3/internal/Util.java:
##########
@@ -88,8 +88,12 @@ public static void debuggerCheck(final JexlEngine ijexl)
throws Exception {
parser.allowRegisters(true);
final Debugger dbg = new Debugger();
// iterate over all expression in
- for (final Map.Entry<Source, ASTJexlScript> entry :
jexl.cache.entries()) {
- final JexlNode node = entry.getValue();
+ for (final Map.Entry<Source, Object> entry : jexl.cache.entries()) {
+ Object c = entry.getValue();
+ if (!(c instanceof JexlNode)) {
+ continue;
Review Comment:
"continue;" needs a comment?
--
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]
