ruthst00 commented on code in PR #6731:
URL: https://github.com/apache/jmeter/pull/6731#discussion_r4066392564
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/control/CacheManager.java:
##########
@@ -602,15 +602,18 @@ public void clear(){
private void clearCache() {
log.debug("Clear cache");
- // TODO: avoid re-creating the thread local every time, reset its
contents instead
- threadCache = new InheritableThreadLocal<Cache<String, CacheEntry>>(){
- @Override
- protected Cache<String, CacheEntry> initialValue() {
- return Caffeine.newBuilder()
- .maximumSize(getMaxSize())
- .build();
- }
- };
+ if (threadCache == null) {
+ threadCache = new InheritableThreadLocal<Cache<String,
CacheEntry>>(){
+ @Override
+ protected Cache<String, CacheEntry> initialValue() {
+ return Caffeine.newBuilder()
+ .maximumSize(getMaxSize())
+ .build();
+ }
+ };
+ } else {
+ threadCache.remove();
Review Comment:
#### The fix is semantically wrong — `threadCache.remove()` does not clear
the cache for all threads
`InheritableThreadLocal.remove()` only removes the value for the __calling
thread__. In JMeter's execution model, `clearCache()` is called from the
engine/controller thread (e.g., in `testIterationStart`), but the cache is
*read and written* by worker threads. Calling `threadCache.remove()` on the
controller thread removes only the controller thread's entry — all worker
threads retain their stale cache entries. The old approach of replacing the
`InheritableThreadLocal` instance was the only way to effectively invalidate
all threads' caches at once (since threads that haven't yet called `.get()` on
the new instance will lazily create a fresh cache via `initialValue()`).
This is a fundamental misunderstanding of how `InheritableThreadLocal` works
in a multi-threaded test engine. The original code's approach — while wasteful
— was actually correct in its semantics.
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/control/CacheManager.java:
##########
@@ -602,15 +602,18 @@ public void clear(){
private void clearCache() {
log.debug("Clear cache");
- // TODO: avoid re-creating the thread local every time, reset its
contents instead
- threadCache = new InheritableThreadLocal<Cache<String, CacheEntry>>(){
- @Override
- protected Cache<String, CacheEntry> initialValue() {
- return Caffeine.newBuilder()
- .maximumSize(getMaxSize())
- .build();
- }
- };
+ if (threadCache == null) {
+ threadCache = new InheritableThreadLocal<Cache<String,
CacheEntry>>(){
+ @Override
+ protected Cache<String, CacheEntry> initialValue() {
+ return Caffeine.newBuilder()
+ .maximumSize(getMaxSize())
+ .build();
+ }
+ };
+ } else {
+ threadCache.remove();
+ }
Review Comment:
### Recommended Fix
The correct approach to address the original TODO without breaking semantics
is to keep the single `InheritableThreadLocal` instance but invalidate the
cache contents rather than replacing the instance. For example:
```java
// Initialize once as a static-like field initializer or in the constructor
only
private transient InheritableThreadLocal<Cache<String, CacheEntry>>
threadCache =
new InheritableThreadLocal<Cache<String, CacheEntry>>() {
@Override
protected Cache<String, CacheEntry> initialValue() {
return Caffeine.newBuilder()
.maximumSize(getMaxSize())
.build();
}
};
private void clearCache() {
log.debug("Clear cache");
Cache<String, CacheEntry> cache = threadCache.get();
if (cache != null) {
cache.invalidateAll();
}
}
```
This keeps the `InheritableThreadLocal` instance stable (no memory leak from
repeated allocation), and `invalidateAll()` clears the Caffeine cache contents
for the current thread — which is the correct scope, since each thread owns its
own cache instance.
##########
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/control/CacheManager.java:
##########
@@ -602,15 +602,18 @@ public void clear(){
private void clearCache() {
log.debug("Clear cache");
- // TODO: avoid re-creating the thread local every time, reset its
contents instead
- threadCache = new InheritableThreadLocal<Cache<String, CacheEntry>>(){
- @Override
- protected Cache<String, CacheEntry> initialValue() {
- return Caffeine.newBuilder()
- .maximumSize(getMaxSize())
- .build();
- }
- };
+ if (threadCache == null) {
Review Comment:
#### The `null` check is unnecessary and misleading
`threadCache` is initialized in the constructor via `clearCache()`, so it
will never be `null` when `clearCache()` is called subsequently. The `if
(threadCache == null)` branch is dead code after construction, which makes the
logic confusing and harder to reason about.
--
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]
