Skip to content

LDEV-6503 (6.2) discard timed-out connections instead of returning them to the pool - #2854

Closed
GianTschingt wants to merge 1 commit into
lucee:6.2from
GianTschingt:LDEV-6503-pool-timeout-6.2
Closed

GianTschingt wants to merge 1 commit into
lucee:6.2from
GianTschingt:LDEV-6503-pool-timeout-6.2

Conversation

@GianTschingt

Copy link
Copy Markdown
Contributor

Jira: https://luceeserver.atlassian.net/browse/LDEV-6503
Forum report: https://dev.lucee.org/t/17621

Problem

Since LDEV-5966 (3417970d4, 6.2.5.6 / 7.0.2.10) DatasourceManagerImpl.releaseConnection() always returns the connection to the pool with release(). Before that, a connection used by a timed-out request was closed. The idea was that validation on the next borrow catches a bad connection. But with validate=false (the default) validateObject() only checks isClosed() and the idle/live timeout.

On 6.2 and 7.0 a request timeout interrupts the thread and then calls Thread.stop(). The ThreadDeath hits as soon as the driver's socket read returns, so the response is left half read on the connection. The connection goes back to the pool and the next request that borrows it gets the result of the timed-out query instead of its own. On a server-level datasource that can be another application or user.

Fix

Same change as the 7.0 PR (6.2 is no longer merged into 7.0, so this is its own PR). In releaseConnection(), if the current thread is interrupted, or the request (ThreadLocalPageContext.get(pc)) has timed out (getTimeoutStackTrace() != null), the connection is invalidated in the pool with a private invalidate(dc) helper instead of release(). The connection is destroyed through commons-pool2 (invalidateObject()), so the pool counters stay right and the LDEV-5966 fix is kept. One change in one place: Query, QueryLazy, Insert/Update, StoredProc, DBInfo and the transaction end() path all release through this method.

6.2 doesn't have LDEV-6129 yet, so this backports its invalidate(dc) helper (with the 6.2 DatasourceConnectionPro casts) and DatasourceConnectionImpl.getPool() unchanged from 7.0. No classloader changes, no new exception wrapping. Apart from that it reuses ThreadLocalPageContext.get() and PageContextImpl.getTimeoutStackTrace().

Trade-off

  • The interrupt check also covers cfthread action="terminate" (there SystemUtil.stop() gets no PageContext, so no timeout stack trace is set).
  • The cost: if a stale interrupt flag is left on a thread, every release in that request discards its connection. That means extra reconnects, but never wrong data.
  • On Java 20+ (Thread.stop() is gone) the query may have finished cleanly before the release. The connection is still discarded, so it's one reconnect per timed-out request, like before 6.2.5.6.

Test

test/tickets/LDEV6503.cfc (labels postgres,datasource, skipped when no Postgres datasource is configured; CI has one). It uses a datasource with validate=false, records pg_backend_pid(), then runs a cfthread with requesttimeout=1 and pg_sleep(5) that returns A_MARKER,A_ID. After that it asserts:

  1. the next query gets its own columns (PID), not the timed-out request's result (fails without the fix on Java < 20);
  2. the backend pid is different, i.e. the connection of the timed-out request was not reused (fails without the fix on every Java version, including CI's);
  3. the pool's active count is back to where it started (LDEV-5966 accounting), and a further query returns the right result.

Repro summary

The forum repro (request A in a cfthread with requesttimeout=3, then request B on the same datasource), PostgreSQL 17, bundled pgjdbc, validate=false, 6 runs each:

Version Java Result
6.2.8.20 17 6/6 swapped (B got A_MARKER,A_ID)
6.2.9.4-RC 17 5/6 swapped
7.0.6.9-RC 17 5/6 swapped
6.2.9.4-RC, 7.0.6.9-RC, 7.1.1.15-RC, 8.0 snapshot 21 no swap, but the same connection is reused after every timeout
with this fix 17, 21 0 swaps, a new backend pid after every timed-out request

validate=true on the datasource avoided the swap in all runs.

Local verification

The changed classes were compiled from this branch (javac --release 11) into the 6.2.9.4-RC jar (the branch base is that RC plus the version bump). LDEV6503.cfc was run with script-runner against a local PostgreSQL 17:

  • unpatched, Java 17: fails, Expected [PID] Actual [A_MARKER,A_ID]
  • unpatched, Java 21: fails, same backend pid
  • with this PR, Java 17 and 21: passes (about 5 s)

Opened by Gian Tschingt, Lucee community assistant for Michael Offner.

…the pool (6.2)

- DatasourceManagerImpl.releaseConnection() invalidates the connection in the
  pool instead of release() when the thread is interrupted or the request timed
  out; a request timeout can stop the driver in the middle of reading a response
  and the next borrower would get that unread result
- the pool accounting from LDEV-5966 is kept, the connection is destroyed
  through the pool
- backports the invalidate() helper (LDEV-6129) and
  DatasourceConnectionImpl.getPool() from 7.0
- test/tickets/LDEV6503.cfc
@GianTschingt

Copy link
Copy Markdown
Contributor Author

Closing this one. By default fixes go into 7.1 only, see #2853. We can still backport later if it's needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant