Skip to content

Commit 83dfbfa

Browse files
adinaueradinauer
committed
fix(spring): Fix get(key, type) double-call in SentryCacheWrapper
Use a single delegate.get(key, type) call instead of calling delegate.get(key) for hit detection and delegate.get(key, type) for the actual value. This eliminates doubled cache round trips (e.g. Redis network calls) and a TOCTOU race where the entry could expire between the two calls. The trade-off is that cached null values are now indistinguishable from cache misses, which is acceptable for observability purposes.
1 parent 518e465 commit 83dfbfa

File tree

6 files changed

+3
-63
lines changed

6 files changed

+3
-63
lines changed

‎sentry-spring-7/src/main/java/io/sentry/spring7/cache/SentryCacheWrapper.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,8 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes
6666
return delegate.get(key, type);
6767
}
6868
try {
69-
final ValueWrapper wrapper = delegate.get(key);
70-
span.setData(SpanDataConvention.CACHE_HIT, wrapper != null);
7169
final T result = delegate.get(key, type);
70+
span.setData(SpanDataConvention.CACHE_HIT, result != null);
7271
span.setStatus(SpanStatus.OK);
7372
return result;
7473
} catch (Throwable e) {

‎sentry-spring-7/src/test/kotlin/io/sentry/spring7/cache/SentryCacheWrapperTest.kt‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,6 @@ class SentryCacheWrapperTest {
8686
fun `get with type creates span with cache hit true on hit`() {
8787
val tx = createTransaction()
8888
val wrapper = SentryCacheWrapper(delegate, scopes)
89-
val valueWrapper = mock<Cache.ValueWrapper>()
90-
whenever(delegate.get("myKey")).thenReturn(valueWrapper)
9189
whenever(delegate.get("myKey", String::class.java)).thenReturn("value")
9290

9391
val result = wrapper.get("myKey", String::class.java)
@@ -97,26 +95,10 @@ class SentryCacheWrapperTest {
9795
assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT))
9896
}
9997

100-
@Test
101-
fun `get with type creates span with cache hit true when cached value is null`() {
102-
val tx = createTransaction()
103-
val wrapper = SentryCacheWrapper(delegate, scopes)
104-
val valueWrapper = mock<Cache.ValueWrapper>()
105-
whenever(delegate.get("myKey")).thenReturn(valueWrapper)
106-
whenever(delegate.get("myKey", String::class.java)).thenReturn(null)
107-
108-
val result = wrapper.get("myKey", String::class.java)
109-
110-
assertNull(result)
111-
assertEquals(1, tx.spans.size)
112-
assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT))
113-
}
114-
11598
@Test
11699
fun `get with type creates span with cache hit false on miss`() {
117100
val tx = createTransaction()
118101
val wrapper = SentryCacheWrapper(delegate, scopes)
119-
whenever(delegate.get("myKey")).thenReturn(null)
120102
whenever(delegate.get("myKey", String::class.java)).thenReturn(null)
121103

122104
val result = wrapper.get("myKey", String::class.java)
@@ -131,7 +113,6 @@ class SentryCacheWrapperTest {
131113
val tx = createTransaction()
132114
val wrapper = SentryCacheWrapper(delegate, scopes)
133115
val exception = RuntimeException("cache error")
134-
whenever(delegate.get("myKey")).thenReturn(mock<Cache.ValueWrapper>())
135116
whenever(delegate.get("myKey", String::class.java)).thenThrow(exception)
136117

137118
assertFailsWith<RuntimeException> { wrapper.get("myKey", String::class.java) }

‎sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/cache/SentryCacheWrapper.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,8 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes
6666
return delegate.get(key, type);
6767
}
6868
try {
69-
final ValueWrapper wrapper = delegate.get(key);
70-
span.setData(SpanDataConvention.CACHE_HIT, wrapper != null);
7169
final T result = delegate.get(key, type);
70+
span.setData(SpanDataConvention.CACHE_HIT, result != null);
7271
span.setStatus(SpanStatus.OK);
7372
return result;
7473
} catch (Throwable e) {

‎sentry-spring-jakarta/src/test/kotlin/io/sentry/spring/jakarta/cache/SentryCacheWrapperTest.kt‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,6 @@ class SentryCacheWrapperTest {
8686
fun `get with type creates span with cache hit true on hit`() {
8787
val tx = createTransaction()
8888
val wrapper = SentryCacheWrapper(delegate, scopes)
89-
val valueWrapper = mock<Cache.ValueWrapper>()
90-
whenever(delegate.get("myKey")).thenReturn(valueWrapper)
9189
whenever(delegate.get("myKey", String::class.java)).thenReturn("value")
9290

9391
val result = wrapper.get("myKey", String::class.java)
@@ -97,26 +95,10 @@ class SentryCacheWrapperTest {
9795
assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT))
9896
}
9997

100-
@Test
101-
fun `get with type creates span with cache hit true when cached value is null`() {
102-
val tx = createTransaction()
103-
val wrapper = SentryCacheWrapper(delegate, scopes)
104-
val valueWrapper = mock<Cache.ValueWrapper>()
105-
whenever(delegate.get("myKey")).thenReturn(valueWrapper)
106-
whenever(delegate.get("myKey", String::class.java)).thenReturn(null)
107-
108-
val result = wrapper.get("myKey", String::class.java)
109-
110-
assertNull(result)
111-
assertEquals(1, tx.spans.size)
112-
assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT))
113-
}
114-
11598
@Test
11699
fun `get with type creates span with cache hit false on miss`() {
117100
val tx = createTransaction()
118101
val wrapper = SentryCacheWrapper(delegate, scopes)
119-
whenever(delegate.get("myKey")).thenReturn(null)
120102
whenever(delegate.get("myKey", String::class.java)).thenReturn(null)
121103

122104
val result = wrapper.get("myKey", String::class.java)
@@ -131,7 +113,6 @@ class SentryCacheWrapperTest {
131113
val tx = createTransaction()
132114
val wrapper = SentryCacheWrapper(delegate, scopes)
133115
val exception = RuntimeException("cache error")
134-
whenever(delegate.get("myKey")).thenReturn(mock<Cache.ValueWrapper>())
135116
whenever(delegate.get("myKey", String::class.java)).thenThrow(exception)
136117

137118
assertFailsWith<RuntimeException> { wrapper.get("myKey", String::class.java) }

‎sentry-spring/src/main/java/io/sentry/spring/cache/SentryCacheWrapper.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,8 @@ public SentryCacheWrapper(final @NotNull Cache delegate, final @NotNull IScopes
6464
return delegate.get(key, type);
6565
}
6666
try {
67-
final ValueWrapper wrapper = delegate.get(key);
68-
span.setData(SpanDataConvention.CACHE_HIT, wrapper != null);
6967
final T result = delegate.get(key, type);
68+
span.setData(SpanDataConvention.CACHE_HIT, result != null);
7069
span.setStatus(SpanStatus.OK);
7170
return result;
7271
} catch (Throwable e) {

‎sentry-spring/src/test/kotlin/io/sentry/spring/cache/SentryCacheWrapperTest.kt‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,6 @@ class SentryCacheWrapperTest {
8484
fun `get with type creates span with cache hit true on hit`() {
8585
val tx = createTransaction()
8686
val wrapper = SentryCacheWrapper(delegate, scopes)
87-
val valueWrapper = mock<Cache.ValueWrapper>()
88-
whenever(delegate.get("myKey")).thenReturn(valueWrapper)
8987
whenever(delegate.get("myKey", String::class.java)).thenReturn("value")
9088

9189
val result = wrapper.get("myKey", String::class.java)
@@ -95,26 +93,10 @@ class SentryCacheWrapperTest {
9593
assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT))
9694
}
9795

98-
@Test
99-
fun `get with type creates span with cache hit true when cached value is null`() {
100-
val tx = createTransaction()
101-
val wrapper = SentryCacheWrapper(delegate, scopes)
102-
val valueWrapper = mock<Cache.ValueWrapper>()
103-
whenever(delegate.get("myKey")).thenReturn(valueWrapper)
104-
whenever(delegate.get("myKey", String::class.java)).thenReturn(null)
105-
106-
val result = wrapper.get("myKey", String::class.java)
107-
108-
assertNull(result)
109-
assertEquals(1, tx.spans.size)
110-
assertEquals(true, tx.spans.first().getData(SpanDataConvention.CACHE_HIT))
111-
}
112-
11396
@Test
11497
fun `get with type creates span with cache hit false on miss`() {
11598
val tx = createTransaction()
11699
val wrapper = SentryCacheWrapper(delegate, scopes)
117-
whenever(delegate.get("myKey")).thenReturn(null)
118100
whenever(delegate.get("myKey", String::class.java)).thenReturn(null)
119101

120102
val result = wrapper.get("myKey", String::class.java)
@@ -129,7 +111,6 @@ class SentryCacheWrapperTest {
129111
val tx = createTransaction()
130112
val wrapper = SentryCacheWrapper(delegate, scopes)
131113
val exception = RuntimeException("cache error")
132-
whenever(delegate.get("myKey")).thenReturn(mock<Cache.ValueWrapper>())
133114
whenever(delegate.get("myKey", String::class.java)).thenThrow(exception)
134115

135116
assertFailsWith<RuntimeException> { wrapper.get("myKey", String::class.java) }

0 commit comments

Comments
 (0)