pjfanning opened a new pull request, #488:
URL: https://github.com/apache/pekko-persistence-r2dbc/pull/488
### Motivation
`R2dbcExecutor` can leak pooled connections when the first call it makes on
a freshly acquired
connection fails.
* `withConnection` schedules the `close-calls-exceeding` watchdog *inside*
the `flatMap` on
`beginTransaction()`. If `beginTransaction()` fails, that block never
runs, so neither the
watchdog nor the `result.failed` / `result.flatMap` close handlers exist,
and the connection is
never returned to the pool. This is reachable whenever the pool hands out
a connection the
server has already dropped (idle timeout, failover, restart), because
`BEGIN` is then the first
statement to fail. Every occurrence permanently burns one slot of
`max-size`, so a backend blip
degrades into pool exhaustion. All transactional work goes through this
path: journal writes and
deletes, `R2dbcSession.withSession`, and the migration tool.
* `withAutoCommitConnection` has the same shape for a failing
`setAutoCommit(true)`. There the
watchdog is already scheduled first, so the connection is held for
`close-calls-exceeding`
(20 seconds by default) rather than forever — and forever when
`close-calls-exceeding = off`.
* `select` and `withAutoCommitConnection` call `connection.close()`
unguarded, while
`commitAndClose` / `rollbackAndClose` wrap the same call in
`ignoreConnectionClosedException`
because `io.r2dbc.pool.PooledConnection` throws `IllegalStateException`
synchronously from
`assertNotClosed` when the connection was already closed. In `select` that
throw happens inside
`mappedRows.flatMap`, so a query whose rows were already read fails with
`IllegalStateException`
instead of returning them whenever `closeAfterTimeout` wins the race.
### Modification
* `withConnection`: schedule the timeout task before `beginTransaction()`,
and close the
connection if `beginTransaction()` fails (no transaction was started, so
there is nothing to
roll back).
* `withAutoCommitConnection`: close the connection if `setAutoCommit(true)`
fails.
* Add a private `closeConnection` helper that applies the existing
`ignoreConnectionClosedException`
catcher, and use it for every close in `select`,
`withAutoCommitConnection`, `commitAndClose` and
`rollbackAndClose`.
No API or binary shape change — only method bodies plus one private method.
### Result
A connection is released back to the pool whichever of `beginTransaction`,
`setAutoCommit` or the
caller's function fails, and a `select` that raced the close watchdog
returns its rows instead of
failing with `IllegalStateException`.
### Tests
- `sbt "core/testOnly
org.apache.pekko.persistence.r2dbc.internal.R2dbcExecutorConnectionReleaseSpec"`
- 3 succeeded
- Same spec against the unpatched `R2dbcExecutor` - 3 failed, confirming the
tests are directional:
`closeCount` stayed at `0` for the two leaks, and the select failed with
`IllegalStateException: Connection is closed` instead of returning its
rows.
- `sbt "core/mimaReportBinaryIssues"` - success (scoped to the only module
touched)
- `sbt "++3.3.8 core/Test/compile"` - success
- `scalafmt --non-interactive` on both changed files - no further changes
- The DB-backed `R2dbcExecutorSpec` and the rest of the suite were not run;
no PostgreSQL or
YugabyteDB instance was available in this environment. The new spec uses a
fake
`ConnectionFactory` and needs no database.
### References
None - connection leak found while auditing resource handling in the main
sources.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
