From 0f7d5c5478f32aa55b33d09d96643fad207bc961 Mon Sep 17 00:00:00 2001 From: David Rogers Date: Thu, 30 Nov 2023 16:23:06 -0600 Subject: [PATCH 1/2] add failing test "does not return 'the same object' from a cacheGet call" adds hook to inject thread delays for testing purposes --- .../extension/io/cache/redis/RedisCache.java | 15 +++++ ...triesAreCopiedOnReadPriorToWriteCommit.cfc | 62 +++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 tests/NearCacheEntriesAreCopiedOnReadPriorToWriteCommit.cfc diff --git a/source/java/src/lucee/extension/io/cache/redis/RedisCache.java b/source/java/src/lucee/extension/io/cache/redis/RedisCache.java index a0136df..7dcb7f2 100644 --- a/source/java/src/lucee/extension/io/cache/redis/RedisCache.java +++ b/source/java/src/lucee/extension/io/cache/redis/RedisCache.java @@ -77,6 +77,14 @@ public class RedisCache extends CacheSupport implements Command { private final Object token = new Object(); + /** + * Boxed type is to support representing the null case + */ + private Integer __test__writeCommitDelay_ms = null; + public Integer get__test__writeCommitDelay_ms() { + return __test__writeCommitDelay_ms; + } + public RedisCache() { if (async) { // storage = new Storage(this); @@ -101,6 +109,9 @@ public void init(Config config, Struct arguments) throws IOException { this.cl = arguments.getClass().getClassLoader(); if (config == null) config = CFMLEngineFactory.getInstance().getThreadConfig(); + __test__writeCommitDelay_ms = caster.toIntValue(arguments.get("__test__writeCommitDelay_ms", null), 0); + __test__writeCommitDelay_ms = __test__writeCommitDelay_ms <= 0 ? null : __test__writeCommitDelay_ms; + host = caster.toString(arguments.get("host", "localhost"), "localhost"); port = caster.toIntValue(arguments.get("port", null), 6379); @@ -960,6 +971,10 @@ public void run() { synchronized (tokenAddToNear) { if (entries.isEmpty()) tokenAddToNear.wait(); } + + if (cache.get__test__writeCommitDelay_ms() != null) { + Thread.sleep(cache.get__test__writeCommitDelay_ms()); + } } catch (Throwable e) { if (cache.log != null) cache.log.error("redis-cache", e); diff --git a/tests/NearCacheEntriesAreCopiedOnReadPriorToWriteCommit.cfc b/tests/NearCacheEntriesAreCopiedOnReadPriorToWriteCommit.cfc new file mode 100644 index 0000000..f9f4b55 --- /dev/null +++ b/tests/NearCacheEntriesAreCopiedOnReadPriorToWriteCommit.cfc @@ -0,0 +1,62 @@ +component extends="org.lucee.cfml.test.LuceeTestCase" labels="redis" { + + public void function beforeAll(){ + variables.cacheName = "NearCacheEntriesAreCopiedOnReadPriorToWriteCommit" + defineCache(); + } + + public void function afterAll(){ + application action="update" caches={}; + } + + private string function defineCache(){ + var redis = server.getDatasource("redis"); + if ( structCount(redis) eq 0 ) + throw "Redis is not configured?"; + + admin + action="updateCacheConnection" + type="server" + password=server.SERVERADMINPASSWORD + class="lucee.extension.io.cache.redis.simple.RedisCache" + bundleName="redis.extension" + name=cacheName + custom={ + "minIdle":8, + "maxTotal":40, + "maxIdle":24, + "host":redis.server, + "port":redis.port, + "socketTimeout":2000, + "liveTimeout":3600000, + "idleTimeout":60000, + "timeToLiveSeconds":0, + "testOnBorrow":true, + "rnd":1, + "__test__writeCommitDelay_ms": 5000 + }, + default="" + readonly=false + storage=false + remoteClients=""; + } + + function run() { + describe("NearCacheEntriesAreCopiedOnReadPriorToWriteCommit", () => { + it("does not return 'the same object' from a cacheGet call", () => { + var someObj = {v: 42} + var key = "redis-test/#createGuid()#"; + + cachePut(key = key, value = someObj, cacheName = cacheName); + + var fromCache = cacheGet(key = key, cacheName = cacheName); + + expect(fromCache.v).toBe(42); + + fromCache.v += 1; + + expect(someObj.v).toBe(42, "mutating 'fromCache' did not mutate the initial cache source 'someObj'"); + }) + }) + } +} From 410fad97804c9f94f1f8e16ab38be190ad3ee999 Mon Sep 17 00:00:00 2001 From: David Rogers Date: Sun, 9 Apr 2023 07:29:02 -0500 Subject: [PATCH 2/2] copy NearCacheEntry values as-if they had been materialized from cache With goal being not sharing live object refs to the underlying "to-be-cached" objects across NearCacheEntry objects fixes test introduced in prior commit resolves #13 --- .../io/cache/redis/NearCacheEntry.java | 19 +++++++++++++++++++ .../extension/io/cache/redis/RedisCache.java | 9 +++++++-- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/source/java/src/lucee/extension/io/cache/redis/NearCacheEntry.java b/source/java/src/lucee/extension/io/cache/redis/NearCacheEntry.java index c1066c0..40e5009 100644 --- a/source/java/src/lucee/extension/io/cache/redis/NearCacheEntry.java +++ b/source/java/src/lucee/extension/io/cache/redis/NearCacheEntry.java @@ -25,6 +25,25 @@ public NearCacheEntry(byte[] key, Object val, int exp, long count) { this.count = count; } + private NearCacheEntry(byte[] key, Object val, int exp, long count, byte[] serialized) { + this.key = key; + this.val = val; + this.exp = exp; + this.created = System.currentTimeMillis(); + this.count = count; + this.serialized = serialized; + } + + /** + * copy this object, also copying the underlying object as-if it had been materialized from cache (in particular, the underlying object + * is copied such that is no longer a reference to the original underlying object). Note that the serialized byte[] is shared across instances of + * copied NearCacheEntries. + */ + public NearCacheEntry copy(ClassLoader cl) throws IOException { + byte[] bytes = serialized(); + return new NearCacheEntry(key, Coder.evaluate(cl, bytes), exp, count, bytes); + } + @Override public Date created() { return new Date(created); diff --git a/source/java/src/lucee/extension/io/cache/redis/RedisCache.java b/source/java/src/lucee/extension/io/cache/redis/RedisCache.java index 7dcb7f2..2e6b5b9 100644 --- a/source/java/src/lucee/extension/io/cache/redis/RedisCache.java +++ b/source/java/src/lucee/extension/io/cache/redis/RedisCache.java @@ -251,7 +251,7 @@ public CacheEntry getCacheEntry(String skey) throws IOException { if (async) { NearCacheEntry val = storage.get(bkey); if (val != null) { - return val; + return val.copy(cl); } storage.doJoin(cnt, true); } @@ -345,7 +345,12 @@ public CacheEntry getCacheEntry(String skey, CacheEntry defaultValue) { if (async) { NearCacheEntry val = storage.get(bkey); if (val != null) { - return val; + try { + return val.copy(cl); + } + catch (IOException e) { + return defaultValue; + } } storage.doJoin(cnt, true); }