Skip to content

Commit 388e24e

Browse files
committed
fix: release the streaming connection and survive a concurrent commit
Two findings from the CodeRabbit review: - When a streaming body threw, the explicit connection.close() inside session.doWork was skipped. session.close() still ran, but the explicit close exists precisely because Spring does not reliably release it, so the failure path could hold database capacity. It is now in a finally, with assertions on both the completing and the throwing path. - dropStagedStreamingHeaders checked isCommitted and then iterated the headers before calling reset(). On the timeout path a stream that started just as the timeout fired can commit inside that window, and reset() would throw IllegalStateException out of the @ExceptionHandler — replacing the intended 503 with a container 500 page. It now falls back to leaving the response alone.
1 parent 1e1b3f6 commit 388e24e

3 files changed

Lines changed: 16 additions & 4 deletions

File tree

backend/app/src/main/kotlin/io/tolgee/ExceptionHandlers.kt

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,14 @@ class ExceptionHandlers(
318318
response.headerNames
319319
.filterNot { STAGED_STREAMING_HEADERS.contains(it) }
320320
.associateWith { response.getHeaders(it).toList() }
321-
response.reset()
321+
try {
322+
response.reset()
323+
} catch (e: IllegalStateException) {
324+
// A stream that started just as the timeout fired can commit between the check above and
325+
// here. Throwing out of an @ExceptionHandler would replace the 503 with a container 500 page.
326+
logger.debug("Response committed while answering a streaming timeout", e)
327+
return
328+
}
322329
preserved.forEach { (name, values) -> values.forEach { response.addHeader(name, it) } }
323330
}
324331

backend/app/src/test/kotlin/io/tolgee/util/StreamingResponseBodyProviderMetricsTest.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ class StreamingResponseBodyProviderMetricsTest {
4848

4949
timerCount(StreamType.IMPORT_APPLY).assert.isEqualTo(1L)
5050
Mockito.verify(session).close()
51+
Mockito.verify(connection).close()
5152
}
5253

5354
@Test
@@ -57,6 +58,7 @@ class StreamingResponseBodyProviderMetricsTest {
5758
.writeTo(ByteArrayOutputStream())
5859

5960
Mockito.verify(session).close()
61+
Mockito.verify(connection).close()
6062
}
6163

6264
@Test

backend/data/src/main/kotlin/io/tolgee/util/StreamingResponseBodyProvider.kt

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,12 @@ class StreamingResponseBodyProvider(
5151
val session = entityManager.unwrap(Session::class.java)
5252
try {
5353
session.doWork { connection ->
54-
fn(it)
55-
// Manually dispose the connection because spring has a hard time doing so by itself
56-
connection.close()
54+
try {
55+
fn(it)
56+
} finally {
57+
// Manually dispose the connection because spring has a hard time doing so by itself
58+
connection.close()
59+
}
5760
}
5861
} finally {
5962
session.close()

0 commit comments

Comments
 (0)