You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by "kusalk (via GitHub)" <gi...@apache.org> on 2023/10/13 11:08:45 UTC
[PR] WW-5355 Use LRU cache by default [struts]
kusalk opened a new pull request, #766:
URL: https://github.com/apache/struts/pull/766
WW-5355
--
I see very limited advantage to using the default cache over the LRU one? The eviction process is faster but at the cost of having to repopulate the whole cache from scratch!
Keen to hear other thoughts or anything I may have missed
Perhaps in the future we can move to a more advanced algorithm such as the ones provided by [Caffeine](https://github.com/ben-manes/caffeine/wiki/Efficiency), although I'm not sure how effective they'd be given the cache can be easily attacked.
--
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: commits-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1761343911
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763378573
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [17 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [91.6% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Integrate W-TinyLfu cache and use by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk merged PR #766:
URL: https://github.com/apache/struts/pull/766
--
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: commits-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763620736
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [93.7% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763618099
Thanks @ben-manes, amended!
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359722722
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlDefaultCache.java:
##########
@@ -30,10 +30,10 @@
public class OgnlDefaultCache<Key, Value> implements OgnlCache<Key, Value> {
private final ConcurrentHashMap<Key, Value> ognlCache;
- private final AtomicInteger cacheEvictionLimit = new AtomicInteger(25000);
+ private final AtomicInteger cacheEvictionLimit = new AtomicInteger();
Review Comment:
The overriding is a fair point - I'll move the initialisation of the whole field into the constructor
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -673,9 +673,15 @@ public void copy(final Object from, final Object to, final Map<String, Object> c
* note if exclusions AND inclusions are supplied and not null nothing will get copied.
* @param editable the class (or interface) to restrict property setting to
*/
- public void copy(final Object from, final Object to, final Map<String, Object> context, Collection<String> exclusions, Collection<String> inclusions, Class<?> editable) {
+ public void copy(final Object from,
+ final Object to,
+ final Map<String, Object> context,
+ Collection<String> exclusions,
+ Collection<String> inclusions,
+ Class<?> editable) {
if (from == null || to == null) {
- LOG.warn("Attempting to copy from or to a null source. This is illegal and is bein skipped. This may be due to an error in an OGNL expression, action chaining, or some other event.");
+ LOG.warn(
Review Comment:
Currently, this method is only called by `ChainingInterceptor` within Struts, and from what I can tell, these parameters will never be null. Although it is also a public method. Calling this method with null values is improper usage and the stack trace will provide better context for such an issue to be resolved. The previous warning message wasn't very helpful.
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not exceed
### that size, dropping the oldest (least-recently-used) expressions to add new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
Review Comment:
Yeah I agree that it would be better to have the defaults in the code and properties file be consistent (as well as the accompanying comments in the properties file). I'll amend this.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359880540
##########
core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java:
##########
@@ -125,6 +127,24 @@
*/
public class DefaultConfiguration implements Configuration {
+ public static final Map<String, Object> BOOTSTRAP_CONSTANTS;
Review Comment:
Extracted bootstrap constants into a public static final field for reuse by the default Struts configuration provider and any test code as needed. Adding new required constants was previously a pain
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359917987
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlCaffeineCache.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Copyright 2022 Apache Software Foundation.
+ *
+ * Licensed 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 com.opensymphony.xwork2.ognl;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
+/**
+ * <p>This OGNL Cache implementation is backed by {@link Caffeine} which uses the Window TinyLfu algorithm.</p>
+ *
+ * <p>An appropriate eviction limit should be chosen for your specific application based on factors and requirements
+ * such as:</p>
+ * <ul>
+ * <li>Quantity and complexity of actions</li>
+ * <li>Volume of requests</li>
+ * <li>Rate limits and attack potential/patterns</li>
+ * <li>Memory constraints</li>
+ * </ul>
+ *
+ * @param <K> The type for the cache key entries
+ * @param <V> The type for the cache value entries
+ */
+public class OgnlCaffeineCache<K, V> implements OgnlCache<K, V> {
+
+ private final Cache<K, V> cache;
+ private final int evictionLimit;
+
+ public OgnlCaffeineCache(int evictionLimit, int initialCapacity) {
+ this.evictionLimit = evictionLimit;
+ this.cache = Caffeine.newBuilder().initialCapacity(initialCapacity).maximumSize(evictionLimit).build();
+ }
+
+ @Override
+ public V get(K key) {
+ return cache.getIfPresent(key);
+ }
+
+ @Override
+ public void put(K key, V value) {
+ cache.put(key, value);
+ }
+
+ @Override
+ public void putIfAbsent(K key, V value) {
+ if (cache.getIfPresent(key) == null) {
+ cache.put(key, value);
+ }
+ }
+
+ @Override
+ public int size() {
+ return Math.toIntExact(cache.estimatedSize());
Review Comment:
`cache.asMap().size()`
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1762492433
fwiw, this would change from a concurrent cache to a synchronized one. It looks like the original code replaced an unbounded concurrent map. The performance difference may be unacceptable of explain the concern without benchmarks/profiling.
Caffeine is very resilient to attacks, like hash flooding and being scan resilient. An independent analysis might be of interest, [An evaluation of cache management policies under workloads with malicious requests](https://ieeexplore.ieee.org/abstract/document/8247467/).
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763389989
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [93.9% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1361461252
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlCacheFactory.java:
##########
@@ -19,12 +19,52 @@
* Used by {@link com.opensymphony.xwork2.ognl.OgnlUtil} to create appropriate OGNL
* caches based on configuration.
*
- * @param <Key> The type for the cache key entries
+ * @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
-interface OgnlCacheFactory<Key, Value> {
+public interface OgnlCacheFactory<Key, Value> {
OgnlCache<Key, Value> buildOgnlCache();
- OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, boolean lruCache);
+
+ /**
+ * Note that if {@code lruCache} is {@code false}, the cache type could still be LRU if the default cache type is
+ * configured as such.
+ * @deprecated since 6.4.0, use {@link #buildOgnlCache(int, int, float, CacheType)}
+ */
+ @Deprecated
+ default OgnlCache<Key, Value> buildOgnlCache(int evictionLimit,
+ int initialCapacity,
+ float loadFactor,
+ boolean lruCache) {
+ return buildOgnlCache(evictionLimit,
+ initialCapacity,
+ loadFactor,
+ lruCache ? CacheType.SYNC_LINKED_LRU : getDefaultCacheType());
+ }
+
+ /**
+ * @param evictionLimit maximum capacity of the cache where applicable for cache type chosen
+ * @param initialCapacity initial capacity of the cache where applicable for cache type chosen
+ * @param loadFactor load factor of the cache where applicable for cache type chosen
+ * @param cacheType type of cache to build
+ * @return a new cache instance
+ */
+ OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, CacheType cacheType);
+
int getCacheMaxSize();
- boolean getUseLRUCache();
+
+ /**
+ * @deprecated since 6.4.0
+ */
+ @Deprecated
+ default boolean getUseLRUCache() {
+ return CacheType.SYNC_LINKED_LRU.equals(getDefaultCacheType());
+ }
+
+ CacheType getDefaultCacheType();
+
+ enum CacheType {
+ CONCURRENT_BASIC,
+ SYNC_LINKED_LRU,
+ CAFFEINE_WTLFU
Review Comment:
I was wondering the same thing - I was only maintaining the design introduced in #528.
I simply replaced `struts.ognl.expressionCacheLRUMode=true|false` with `struts.ognl.expressionCacheType=basic|lru|wtlfu`
Let me look into refactoring this, it will likely be a breaking change though.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Integrate W-TinyLfu cache and use by default [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1361697311
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlCacheFactory.java:
##########
@@ -19,12 +19,52 @@
* Used by {@link com.opensymphony.xwork2.ognl.OgnlUtil} to create appropriate OGNL
* caches based on configuration.
*
- * @param <Key> The type for the cache key entries
+ * @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
-interface OgnlCacheFactory<Key, Value> {
+public interface OgnlCacheFactory<Key, Value> {
OgnlCache<Key, Value> buildOgnlCache();
- OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, boolean lruCache);
+
+ /**
+ * Note that if {@code lruCache} is {@code false}, the cache type could still be LRU if the default cache type is
+ * configured as such.
+ * @deprecated since 6.4.0, use {@link #buildOgnlCache(int, int, float, CacheType)}
+ */
+ @Deprecated
+ default OgnlCache<Key, Value> buildOgnlCache(int evictionLimit,
+ int initialCapacity,
+ float loadFactor,
+ boolean lruCache) {
+ return buildOgnlCache(evictionLimit,
+ initialCapacity,
+ loadFactor,
+ lruCache ? CacheType.SYNC_LINKED_LRU : getDefaultCacheType());
+ }
+
+ /**
+ * @param evictionLimit maximum capacity of the cache where applicable for cache type chosen
+ * @param initialCapacity initial capacity of the cache where applicable for cache type chosen
+ * @param loadFactor load factor of the cache where applicable for cache type chosen
+ * @param cacheType type of cache to build
+ * @return a new cache instance
+ */
+ OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, CacheType cacheType);
+
int getCacheMaxSize();
- boolean getUseLRUCache();
+
+ /**
+ * @deprecated since 6.4.0
+ */
+ @Deprecated
+ default boolean getUseLRUCache() {
+ return CacheType.SYNC_LINKED_LRU.equals(getDefaultCacheType());
+ }
+
+ CacheType getDefaultCacheType();
+
+ enum CacheType {
+ CONCURRENT_BASIC,
+ SYNC_LINKED_LRU,
+ CAFFEINE_WTLFU
Review Comment:
Thanks! That would make more sense and then we can remove all the properties and leave that to a given implementation. Please create the ticket :)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359882251
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -143,12 +143,18 @@ protected void setEnableExpressionCache(String cache) {
enableExpressionCache = BooleanUtils.toBoolean(cache);
}
- @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, required = false)
Review Comment:
Doesn't make much sense to inject this both in the factory and here. Plus, ~~Caffeine doesn't support changing the max size after construction~~ (and neither does LRU really), ~~so we'd have to flush/reconstruct the cache to continue supporting this method~~. I can't see much use for changing cache max size mid-life anyway so I've just gone ahead and deprecated it.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763245109
Thank you for the well considered review @JCgH4164838Gh792C124B5
Yep I no longer intend to make LRU (backed by a synchronised LinkedHashMap) the default given the performance implications. And yes I intend to keep the cache implementation configurable, even with the implementation of Caffeine.
I think choosing a default cache size will always be a challenge as it is subject to multiple application-specific factors so the best we can do here is to encourage developers to adjust the cache size as needed.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763396297
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [93.9% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763380211
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [17 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [91.6% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=sonarcloud-welcome)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763383378
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [17 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [93.9% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359879865
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not exceed
### that size, dropping the oldest (least-recently-used) expressions to add new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
Review Comment:
I ended up defining the defaults in the properties file and removing defaults from the code - except for required values when bootstrapping the container. (And except for a deprecated constructor which is awaiting deletion)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359880540
##########
core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java:
##########
@@ -125,6 +127,24 @@
*/
public class DefaultConfiguration implements Configuration {
+ public static final Map<String, Object> BOOTSTRAP_CONSTANTS;
Review Comment:
Extracted bootstrap constants into a public static final field for reuse by the default Struts configuration provider and any test code as needed
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "JCgH4164838Gh792C124B5 (via GitHub)" <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359618422
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlLRUCache.java:
##########
@@ -36,15 +36,15 @@
public class OgnlLRUCache<Key, Value> implements OgnlCache<Key, Value> {
private final Map<Key, Value> ognlLRUCache;
- private final AtomicInteger cacheEvictionLimit = new AtomicInteger(2500);
+ private final AtomicInteger cacheEvictionLimit = new AtomicInteger();
Review Comment:
The `OgnlDefaultCache` comment above would apply here as well.
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not exceed
### that size, dropping the oldest (least-recently-used) expressions to add new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
### Indicates if the OGNL expressionCache should use LRU mode.
### NOTE: When true, make sure to set the expressionCacheMaxSize to a reasonable value
### for your application. Otherwise the default limit will never (practically) be reached.
-# struts.ognl.expressionCacheLRUMode=false
+struts.ognl.expressionCacheLRUMode=true
Review Comment:
@ben-manes already pointed out potential implications in making the LRU mode cache active by default (switching from a concurrent to synchronized cache). It might be safer to keep LRU mode caching off by default, at least until someone can confirm if performance of the LRU mode cache is acceptable under load (benchmarking).
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not exceed
### that size, dropping the oldest (least-recently-used) expressions to add new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
Review Comment:
If an (uncommented) initial value it to be used/set in `default.properties`, maybe it should match the programmatic defaults in the factory and cache instances ?
Whether 10000 (or 25000) is a reasonable default might not be determined without some benchmarks.
This comment would apply to any of the numeric size choices in the properties file.
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -705,29 +710,31 @@ public void copy(final Object from, final Object to, final Map<String, Object> c
}
for (PropertyDescriptor fromPd : fromPds) {
Review Comment:
Looks reasonable to me.
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -673,9 +673,15 @@ public void copy(final Object from, final Object to, final Map<String, Object> c
* note if exclusions AND inclusions are supplied and not null nothing will get copied.
* @param editable the class (or interface) to restrict property setting to
*/
- public void copy(final Object from, final Object to, final Map<String, Object> context, Collection<String> exclusions, Collection<String> inclusions, Class<?> editable) {
+ public void copy(final Object from,
+ final Object to,
+ final Map<String, Object> context,
+ Collection<String> exclusions,
+ Collection<String> inclusions,
+ Class<?> editable) {
if (from == null || to == null) {
- LOG.warn("Attempting to copy from or to a null source. This is illegal and is bein skipped. This may be due to an error in an OGNL expression, action chaining, or some other event.");
+ LOG.warn(
Review Comment:
It looks like the wording of that log warning has been invariant as far back as the history for the file goes. Does changing it by reducing the wording detail (other than maybe fixing the "is bein" typo) make it less clear ?
I am unsure of how frequently this log statement might get triggered, but adding "`new RuntimeException()`" as a parameter would (I think ?) result in a stack-trace being output each time. That might potentially create a lot of log clutter (a trade-off, if the stack-trace gives better context) ?
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlDefaultCache.java:
##########
@@ -30,10 +30,10 @@
public class OgnlDefaultCache<Key, Value> implements OgnlCache<Key, Value> {
private final ConcurrentHashMap<Key, Value> ognlCache;
- private final AtomicInteger cacheEvictionLimit = new AtomicInteger(25000);
+ private final AtomicInteger cacheEvictionLimit = new AtomicInteger();
Review Comment:
I think setting a positive non-zero value for the initializer was to avoid the instance ever having a `cacheEvictionLimit` of zero (which "`new AtomicInteger()`" would). Also, if someone built a custom cache implementation extending `OgnlDefaultCache` with an overridden constructor that did not call `super()`, a (hopefully) reasonable nonzero initial value would be ensured.
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not exceed
### that size, dropping the oldest (least-recently-used) expressions to add new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
### Indicates if the OGNL expressionCache should use LRU mode.
### NOTE: When true, make sure to set the expressionCacheMaxSize to a reasonable value
### for your application. Otherwise the default limit will never (practically) be reached.
-# struts.ognl.expressionCacheLRUMode=false
+struts.ognl.expressionCacheLRUMode=true
### Specify a limit to the number of entries in the OGNL beanInfoCache.
### For the standard beanInfoCache mode, when the limit is exceeded the entire cache's
### content will be cleared (can help prevent memory leaks).
### For beanInfoCacheLRUMode true, the limit will ensure the cache does not exceed
### that size, dropping the oldest (least-recently-used) expressions to add new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.beanInfoCacheMaxSize=1000
+struts.ognl.beanInfoCacheMaxSize=5000
### Indicates if the OGNL beanInfoCache should use LRU mode.
### NOTE: When true, make sure to set the beanInfoCacheMaxSize to a reasonable value
### for your application. Otherwise the default limit will never (practically) be reached.
-# struts.ognl.beanInfoCacheLRUMode=false
+struts.ognl.beanInfoCacheLRUMode=true
Review Comment:
Above comment on LRU mode defaults applies here as well.
##########
core/src/main/resources/org/apache/struts2/default.properties:
##########
@@ -243,25 +243,25 @@ struts.ognl.enableExpressionCache=true
### For expressionCacheLRUMode true, the limit will ensure the cache does not exceed
### that size, dropping the oldest (least-recently-used) expressions to add new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.expressionCacheMaxSize=1000
+struts.ognl.expressionCacheMaxSize=10000
### Indicates if the OGNL expressionCache should use LRU mode.
### NOTE: When true, make sure to set the expressionCacheMaxSize to a reasonable value
### for your application. Otherwise the default limit will never (practically) be reached.
-# struts.ognl.expressionCacheLRUMode=false
+struts.ognl.expressionCacheLRUMode=true
### Specify a limit to the number of entries in the OGNL beanInfoCache.
### For the standard beanInfoCache mode, when the limit is exceeded the entire cache's
### content will be cleared (can help prevent memory leaks).
### For beanInfoCacheLRUMode true, the limit will ensure the cache does not exceed
### that size, dropping the oldest (least-recently-used) expressions to add new ones.
### NOTE: If not set, the default is 25000, which may be excessive.
-# struts.ognl.beanInfoCacheMaxSize=1000
+struts.ognl.beanInfoCacheMaxSize=5000
Review Comment:
Above comment for numeric defaults applies here as well.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359882251
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -143,12 +143,18 @@ protected void setEnableExpressionCache(String cache) {
enableExpressionCache = BooleanUtils.toBoolean(cache);
}
- @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, required = false)
Review Comment:
Doesn't make much sense to inject this both in the factory and here. Plus, Caffeine doesn't support changing the max size after construction (and neither does LRU really), so we'd have to flush/reconstruct the cache to continue supporting this method. I can't see much use for it anyway so I've just gone ahead and deprecated it.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1360728599
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlCacheFactory.java:
##########
@@ -19,12 +19,52 @@
* Used by {@link com.opensymphony.xwork2.ognl.OgnlUtil} to create appropriate OGNL
* caches based on configuration.
*
- * @param <Key> The type for the cache key entries
+ * @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
-interface OgnlCacheFactory<Key, Value> {
+public interface OgnlCacheFactory<Key, Value> {
OgnlCache<Key, Value> buildOgnlCache();
- OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, boolean lruCache);
+
+ /**
+ * Note that if {@code lruCache} is {@code false}, the cache type could still be LRU if the default cache type is
+ * configured as such.
+ * @deprecated since 6.4.0, use {@link #buildOgnlCache(int, int, float, CacheType)}
+ */
+ @Deprecated
+ default OgnlCache<Key, Value> buildOgnlCache(int evictionLimit,
+ int initialCapacity,
+ float loadFactor,
+ boolean lruCache) {
+ return buildOgnlCache(evictionLimit,
+ initialCapacity,
+ loadFactor,
+ lruCache ? CacheType.SYNC_LINKED_LRU : getDefaultCacheType());
+ }
+
+ /**
+ * @param evictionLimit maximum capacity of the cache where applicable for cache type chosen
+ * @param initialCapacity initial capacity of the cache where applicable for cache type chosen
+ * @param loadFactor load factor of the cache where applicable for cache type chosen
+ * @param cacheType type of cache to build
+ * @return a new cache instance
+ */
+ OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, CacheType cacheType);
+
int getCacheMaxSize();
- boolean getUseLRUCache();
+
+ /**
+ * @deprecated since 6.4.0
+ */
+ @Deprecated
+ default boolean getUseLRUCache() {
+ return CacheType.SYNC_LINKED_LRU.equals(getDefaultCacheType());
+ }
+
+ CacheType getDefaultCacheType();
+
+ enum CacheType {
+ CONCURRENT_BASIC,
+ SYNC_LINKED_LRU,
+ CAFFEINE_WTLFU
Review Comment:
Are these different implementations of cache layer? Shouldn't this go through the extension point?
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1765603308
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [90.1% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763391582
I found that previously, `struts.ognl.expressionCacheLRUMode=true` was not reliable due to setter injection not guaranteeing the order of operations. I've thus refactored the code to use constructor injection.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359917925
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlCaffeineCache.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Copyright 2022 Apache Software Foundation.
+ *
+ * Licensed 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 com.opensymphony.xwork2.ognl;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
+/**
+ * <p>This OGNL Cache implementation is backed by {@link Caffeine} which uses the Window TinyLfu algorithm.</p>
+ *
+ * <p>An appropriate eviction limit should be chosen for your specific application based on factors and requirements
+ * such as:</p>
+ * <ul>
+ * <li>Quantity and complexity of actions</li>
+ * <li>Volume of requests</li>
+ * <li>Rate limits and attack potential/patterns</li>
+ * <li>Memory constraints</li>
+ * </ul>
+ *
+ * @param <K> The type for the cache key entries
+ * @param <V> The type for the cache value entries
+ */
+public class OgnlCaffeineCache<K, V> implements OgnlCache<K, V> {
+
+ private final Cache<K, V> cache;
+ private final int evictionLimit;
+
+ public OgnlCaffeineCache(int evictionLimit, int initialCapacity) {
+ this.evictionLimit = evictionLimit;
+ this.cache = Caffeine.newBuilder().initialCapacity(initialCapacity).maximumSize(evictionLimit).build();
+ }
+
+ @Override
+ public V get(K key) {
+ return cache.getIfPresent(key);
+ }
+
+ @Override
+ public void put(K key, V value) {
+ cache.put(key, value);
+ }
+
+ @Override
+ public void putIfAbsent(K key, V value) {
+ if (cache.getIfPresent(key) == null) {
+ cache.put(key, value);
+ }
Review Comment:
`cache.asMap().putIfAbsent(key, value)`
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359882251
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -143,12 +143,18 @@ protected void setEnableExpressionCache(String cache) {
enableExpressionCache = BooleanUtils.toBoolean(cache);
}
- @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, required = false)
Review Comment:
Doesn't make much sense to inject this both in the factory and here. Plus, Caffeine doesn't support changing the max size after construction (and neither does LRU really), so we'd have to flush/reconstruct the cache to continue supporting this method. I can't see much use for changing cache max size mid-life anyway so I've just gone ahead and deprecated it.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1361461252
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlCacheFactory.java:
##########
@@ -19,12 +19,52 @@
* Used by {@link com.opensymphony.xwork2.ognl.OgnlUtil} to create appropriate OGNL
* caches based on configuration.
*
- * @param <Key> The type for the cache key entries
+ * @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
-interface OgnlCacheFactory<Key, Value> {
+public interface OgnlCacheFactory<Key, Value> {
OgnlCache<Key, Value> buildOgnlCache();
- OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, boolean lruCache);
+
+ /**
+ * Note that if {@code lruCache} is {@code false}, the cache type could still be LRU if the default cache type is
+ * configured as such.
+ * @deprecated since 6.4.0, use {@link #buildOgnlCache(int, int, float, CacheType)}
+ */
+ @Deprecated
+ default OgnlCache<Key, Value> buildOgnlCache(int evictionLimit,
+ int initialCapacity,
+ float loadFactor,
+ boolean lruCache) {
+ return buildOgnlCache(evictionLimit,
+ initialCapacity,
+ loadFactor,
+ lruCache ? CacheType.SYNC_LINKED_LRU : getDefaultCacheType());
+ }
+
+ /**
+ * @param evictionLimit maximum capacity of the cache where applicable for cache type chosen
+ * @param initialCapacity initial capacity of the cache where applicable for cache type chosen
+ * @param loadFactor load factor of the cache where applicable for cache type chosen
+ * @param cacheType type of cache to build
+ * @return a new cache instance
+ */
+ OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, CacheType cacheType);
+
int getCacheMaxSize();
- boolean getUseLRUCache();
+
+ /**
+ * @deprecated since 6.4.0
+ */
+ @Deprecated
+ default boolean getUseLRUCache() {
+ return CacheType.SYNC_LINKED_LRU.equals(getDefaultCacheType());
+ }
+
+ CacheType getDefaultCacheType();
+
+ enum CacheType {
+ CONCURRENT_BASIC,
+ SYNC_LINKED_LRU,
+ CAFFEINE_WTLFU
Review Comment:
I was wondering the same thing - I was only maintaining the design introduced in #528.
I simply replaced `struts.ognl.expressionCacheLRUMode=true|false` with `struts.ognl.expressionCacheType=concurrent_basic|sync_linked_lru|caffeine_wtlfu`
Let me look into refactoring this, it will likely be a breaking change though.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1765614800
@lukaszlenart I've updated the PR description :)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359881125
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlLRUCache.java:
##########
@@ -81,7 +81,9 @@ public int getEvictionLimit() {
@Override
public void setEvictionLimit(int cacheEvictionLimit) {
+ if (cacheEvictionLimit < size()) {
Review Comment:
Fixed this bug
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1761335425
@JCgH4164838Gh792C124B5
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1358120890
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlDefaultCache.java:
##########
@@ -30,10 +30,10 @@
public class OgnlDefaultCache<Key, Value> implements OgnlCache<Key, Value> {
private final ConcurrentHashMap<Key, Value> ognlCache;
- private final AtomicInteger cacheEvictionLimit = new AtomicInteger(25000);
+ private final AtomicInteger cacheEvictionLimit = new AtomicInteger();
Review Comment:
These init values are inconsequential as far as I can tell as they are immediately overridden by the constructor
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Integrate W-TinyLfu cache and use by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1363905067
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlCacheFactory.java:
##########
@@ -19,12 +19,52 @@
* Used by {@link com.opensymphony.xwork2.ognl.OgnlUtil} to create appropriate OGNL
* caches based on configuration.
*
- * @param <Key> The type for the cache key entries
+ * @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
-interface OgnlCacheFactory<Key, Value> {
+public interface OgnlCacheFactory<Key, Value> {
OgnlCache<Key, Value> buildOgnlCache();
- OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, boolean lruCache);
+
+ /**
+ * Note that if {@code lruCache} is {@code false}, the cache type could still be LRU if the default cache type is
+ * configured as such.
+ * @deprecated since 6.4.0, use {@link #buildOgnlCache(int, int, float, CacheType)}
+ */
+ @Deprecated
+ default OgnlCache<Key, Value> buildOgnlCache(int evictionLimit,
+ int initialCapacity,
+ float loadFactor,
+ boolean lruCache) {
+ return buildOgnlCache(evictionLimit,
+ initialCapacity,
+ loadFactor,
+ lruCache ? CacheType.SYNC_LINKED_LRU : getDefaultCacheType());
+ }
+
+ /**
+ * @param evictionLimit maximum capacity of the cache where applicable for cache type chosen
+ * @param initialCapacity initial capacity of the cache where applicable for cache type chosen
+ * @param loadFactor load factor of the cache where applicable for cache type chosen
+ * @param cacheType type of cache to build
+ * @return a new cache instance
+ */
+ OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, CacheType cacheType);
+
int getCacheMaxSize();
- boolean getUseLRUCache();
+
+ /**
+ * @deprecated since 6.4.0
+ */
+ @Deprecated
+ default boolean getUseLRUCache() {
+ return CacheType.SYNC_LINKED_LRU.equals(getDefaultCacheType());
+ }
+
+ CacheType getDefaultCacheType();
+
+ enum CacheType {
+ CONCURRENT_BASIC,
+ SYNC_LINKED_LRU,
+ CAFFEINE_WTLFU
Review Comment:
[WW-5356](https://issues.apache.org/jira/browse/WW-5356)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1762504552
Good call out @ben-manes, I completely glossed over the change from concurrent map to synchronised map (and its performance implications).
And thank you for that analysis on resistance to attacks.
In that case I may just go ahead and integrate Caffeine as it seems rather straightforward anyway.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use LRU cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1358120083
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -705,29 +710,31 @@ public void copy(final Object from, final Object to, final Map<String, Object> c
}
for (PropertyDescriptor fromPd : fromPds) {
Review Comment:
Nothing to see here, just made this code more digestible
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763387927
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [94.0% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763389644
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [93.9% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359882251
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -143,12 +143,18 @@ protected void setEnableExpressionCache(String cache) {
enableExpressionCache = BooleanUtils.toBoolean(cache);
}
- @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, required = false)
Review Comment:
Doesn't make much sense to inject this both in the factory and here. Plus, Caffeine doesn't support changing the max size after construction (and neither does LRU really), so we'd have to rebuild the cache to continue supporting this method. I can't see much use for it anyway so I've just gone ahead and deprecated it.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763400837
I've taken care to maintain full API compatibility given that these classes are extension points and thus public API
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #766:
URL: https://github.com/apache/struts/pull/766#issuecomment-1763394968
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_struts&pullRequest=766)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_struts&pullRequest=766&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL) [13 Code Smells](https://sonarcloud.io/project/issues?id=apache_struts&pullRequest=766&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list) [93.9% Coverage](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_struts&pullRequest=766&metric=new_duplicated_lines_density&view=list)
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359880961
##########
core/src/main/java/com/opensymphony/xwork2/ognl/DefaultOgnlCacheFactory.java:
##########
@@ -15,52 +15,86 @@
*/
package com.opensymphony.xwork2.ognl;
-import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicInteger;
import org.apache.commons.lang3.BooleanUtils;
/**
- * Default OGNL Cache factory implementation.
+ * <p>Default OGNL Cache factory implementation.</p>
*
- * Currently used for Expression cache and BeanInfo cache creation.
+ * <p>Currently used for Expression cache and BeanInfo cache creation.</p>
*
- * @param <Key> The type for the cache key entries
+ * @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
public class DefaultOgnlCacheFactory<Key, Value> implements OgnlCacheFactory<Key, Value> {
- private final AtomicBoolean useLRUCache = new AtomicBoolean(false);
- private final AtomicInteger cacheMaxSize = new AtomicInteger(25000);
Review Comment:
Making this factory class thread-safe seemed unnecessary
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1359917451
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java:
##########
@@ -143,12 +143,18 @@ protected void setEnableExpressionCache(String cache) {
enableExpressionCache = BooleanUtils.toBoolean(cache);
}
- @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, required = false)
Review Comment:
```java
cache.policy().eviction().ifPresent(eviction -> {
eviction.setMaximum(2 * eviction.getMaximum());
});
```
The [Policy](https://www.javadoc.io/doc/com.github.ben-manes.caffeine/caffeine/latest/com.github.benmanes.caffeine/com/github/benmanes/caffeine/cache/Policy.html) api contains various methods for pragmatic needs, while keeping the core apis simple and focused on the common needs.
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WW-5355 Use W-TinyLfu cache by default [struts]
Posted by "kusalk (via GitHub)" <gi...@apache.org>.
kusalk commented on code in PR #766:
URL: https://github.com/apache/struts/pull/766#discussion_r1361463708
##########
core/src/main/java/com/opensymphony/xwork2/ognl/OgnlCacheFactory.java:
##########
@@ -19,12 +19,52 @@
* Used by {@link com.opensymphony.xwork2.ognl.OgnlUtil} to create appropriate OGNL
* caches based on configuration.
*
- * @param <Key> The type for the cache key entries
+ * @param <Key> The type for the cache key entries
* @param <Value> The type for the cache value entries
*/
-interface OgnlCacheFactory<Key, Value> {
+public interface OgnlCacheFactory<Key, Value> {
OgnlCache<Key, Value> buildOgnlCache();
- OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, boolean lruCache);
+
+ /**
+ * Note that if {@code lruCache} is {@code false}, the cache type could still be LRU if the default cache type is
+ * configured as such.
+ * @deprecated since 6.4.0, use {@link #buildOgnlCache(int, int, float, CacheType)}
+ */
+ @Deprecated
+ default OgnlCache<Key, Value> buildOgnlCache(int evictionLimit,
+ int initialCapacity,
+ float loadFactor,
+ boolean lruCache) {
+ return buildOgnlCache(evictionLimit,
+ initialCapacity,
+ loadFactor,
+ lruCache ? CacheType.SYNC_LINKED_LRU : getDefaultCacheType());
+ }
+
+ /**
+ * @param evictionLimit maximum capacity of the cache where applicable for cache type chosen
+ * @param initialCapacity initial capacity of the cache where applicable for cache type chosen
+ * @param loadFactor load factor of the cache where applicable for cache type chosen
+ * @param cacheType type of cache to build
+ * @return a new cache instance
+ */
+ OgnlCache<Key, Value> buildOgnlCache(int evictionLimit, int initialCapacity, float loadFactor, CacheType cacheType);
+
int getCacheMaxSize();
- boolean getUseLRUCache();
+
+ /**
+ * @deprecated since 6.4.0
+ */
+ @Deprecated
+ default boolean getUseLRUCache() {
+ return CacheType.SYNC_LINKED_LRU.equals(getDefaultCacheType());
+ }
+
+ CacheType getDefaultCacheType();
+
+ enum CacheType {
+ CONCURRENT_BASIC,
+ SYNC_LINKED_LRU,
+ CAFFEINE_WTLFU
Review Comment:
Yeah given the breaking nature, I'm more inclined to take on this refactor as part of 7.0, all good for me to create a follow-up card for it?
--
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: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org