diff --git a/changelog.d/4219-advisory-lock-abort-release.fixed.md b/changelog.d/4219-advisory-lock-abort-release.fixed.md new file mode 100644 index 0000000000..00d2118403 --- /dev/null +++ b/changelog.d/4219-advisory-lock-abort-release.fixed.md @@ -0,0 +1 @@ +- `withAdvisoryLock(transaction=true)` now releases its session-scoped database lock (MySQL, SQL Server) when the callback ends the request with `abort` or `redirectTo()`. The release previously ran in the same step that commits or rolls back the transaction, which some engines skip when an `abort` leaves the transaction, so the lock could stay held on the pooled connection until that connection closed; it now runs in its own step that executes on every engine, while the normal path still commits before releasing so the next holder reads committed state (#4219) diff --git a/changelog.d/4219-testclient-get-timeout.added.md b/changelog.d/4219-testclient-get-timeout.added.md new file mode 100644 index 0000000000..ed492820c1 --- /dev/null +++ b/changelog.d/4219-testclient-get-timeout.added.md @@ -0,0 +1 @@ +- `$testClient().get()` accepts an optional `timeout` (seconds, default 30) that bounds the underlying HTTP request, so a request that may hang fails the asserting test case rather than timing out the whole test run (#4219) diff --git a/vendor/wheels/model/locking.cfm b/vendor/wheels/model/locking.cfm index ad3a660695..034401a259 100644 --- a/vendor/wheels/model/locking.cfm +++ b/vendor/wheels/model/locking.cfm @@ -237,37 +237,61 @@ public boolean function $advisoryLockTransactionBody(required string name, required numeric timeout, required any callback, required struct state) { local.adapter = variables.wheels.class.adapter; local.committed = {flag = false}; - var release = {error = ""}; + var release = {error = "", done = false}; var threw = {flag = false}; + var cb = {done = false}; local.adapter.$acquireAdvisoryLockTransactional(name = arguments.name, timeout = arguments.timeout); try { - local.cbResult = arguments.callback(); - if (StructKeyExists(local, "cbResult")) { - arguments.state.hasResult = true; - arguments.state.result = local.cbResult; + try { + local.cbResult = arguments.callback(); + if (StructKeyExists(local, "cbResult")) { + arguments.state.hasResult = true; + arguments.state.result = local.cbResult; + } + cb.done = true; + } finally { + // This inner finally holds the lock RELEASE and contains NO transaction-action statement, + // so BoxLang runs it even when the callback ends the request with `abort`: a `transaction + // action="…"` inside a finally is skipped by BoxLang when an abort leaves the surrounding + // cftransaction (#4219 — Lucee/Adobe run it either way). On an abnormal exit (abort or a + // throw -> cb.done stays false) the lock is released HERE; on the normal path cb.done is + // true and the commit-then-release below runs instead, preserving commit-before-release so + // the next holder reads committed state. Unscoped struct writes persist past the catch on + // BoxLang (invariant 11). + if (!cb.done) { + try { + $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); + } catch (any releaseErr) { + WriteLog(type = "error", file = "wheels", text = "withAdvisoryLock(transaction=true): release on the abnormal-exit path also failed: " & releaseErr.message); + } + release.done = true; + } } - // Commit on the pinned connection BEFORE the release, so the next holder reads committed - // state. The block stays open on the same connection for the release that follows; - // invokeWithTransaction's own block close then has nothing left to commit. `committed` flips - // only AFTER a clean commit, so a failing commit takes the failure path (rollback). + // Normal path only (abort/throw skipped past this via the inner finally). Commit on the pinned + // connection BEFORE the release, so the next holder reads committed state. `committed` flips + // only AFTER a clean commit, so a failing commit takes the catch/finally path below. transaction action="commit"; local.committed.flag = true; + try { + $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); + } catch (any releaseErr2) { + release.error = releaseErr2; + } + release.done = true; } catch (any e) { - // Mark a caught throw (callback or commit) so the finally does NOT roll back here: on a - // throw, invokeWithTransaction's own catch rolls the transaction back after this method - // returns, so a rollback in the finally would be a redundant second one. Only the ABORT - // path needs the finally to roll back, because an abort is never caught and the wrapper's - // catch never runs (see the finally). Unscoped struct write so it persists out of the catch - // on BoxLang (invariant 11). The exception still propagates to invokeWithTransaction. + // A caught throw (callback or commit). On a throw, invokeWithTransaction's own catch rolls the + // transaction back after this method returns, so the finally does NOT roll back. Unscoped + // struct write persists past the catch on BoxLang (invariant 11). The exception still + // propagates to invokeWithTransaction. threw.flag = true; rethrow; } finally { // Roll back ONLY on the ABORT path — not committed, and not a caught throw. An aborting // callback is never caught, so invokeWithTransaction's catch never runs and the engine would - // otherwise decide the open transaction's fate at block exit (a commit-at-exit would reopen - // the pre-commit window after the lock is freed). The rollback is in its own try/catch: a - // rollback that itself throws (a dead connection the server has already rolled back) must not - // skip the release below. + // otherwise decide the open transaction's fate at block exit. This rollback is a + // transaction-action, and BoxLang skips a finally that contains one when an abort leaves the + // cftransaction; the LOCK release above sits in a separate finally with no transaction-action, + // so it still runs on every engine on abort. Lucee/Adobe run this finally normally. if (!local.committed.flag && !threw.flag) { try { transaction action="rollback"; @@ -275,16 +299,16 @@ WriteLog(type = "error", file = "wheels", text = "withAdvisoryLock(transaction=true): rollback on the abort path failed: " & rollbackErr.message); } } - // Always attempt the release, whatever the rollback did. - try { - $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); - } catch (any releaseErr) { - // Unscoped struct write so it persists out of the catch on BoxLang (invariant 11). - if (local.committed.flag) { - release.error = releaseErr; - } else { - WriteLog(type = "error", file = "wheels", text = "withAdvisoryLock(transaction=true): release after a failed callback/commit also failed: " & releaseErr.message); + // Safety net for a commit failure: a throw AFTER the inner finally leaves cb.done true (so the + // inner finally did not release) and committed false (so the commit threw). A throw runs this + // finally on every engine, so release here if it has not happened yet. + if (!release.done) { + try { + $releaseTransactionalAdvisoryLock(adapter = local.adapter, name = arguments.name); + } catch (any releaseErr3) { + WriteLog(type = "error", file = "wheels", text = "withAdvisoryLock(transaction=true): release after a failed commit also failed: " & releaseErr3.message); } + release.done = true; } } // Reached only on the committed path (a callback / commit failure propagates past here). Hand a diff --git a/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc b/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc new file mode 100644 index 0000000000..6b92be2acb --- /dev/null +++ b/vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc @@ -0,0 +1,59 @@ +/** + * Test-asset controller for the #4219 release-on-abort regression spec + * (advisoryLockAbortReleaseSpec). Each action takes a withAdvisoryLock() lock and then ends the + * request from INSIDE the callback — with a real `abort` or a real `redirectTo()` (which ends in a + * cflocation) — for both the default (session-scoped) and the transaction = true path. + * + * Positive control: while inside the lock, and before ending the request, each callback records a + * held-check taken from a SECOND connection into server["wheelsAdvisoryProbe_"]. The spec + * reads that marker and asserts it is true, so a scenario cannot pass vacuously if the request never + * reached withAdvisoryLock (a 404, a load failure, the wrong datasource, an early throw) — in those + * cases the marker is simply absent. $isAdvisoryLockHeld is a server-wide query, so the separate + * connection proves the lock was genuinely held at the moment of the abort / redirect. + * + * The lock's release runs in a `finally`, which executes on abort / cflocation, so the lock must be + * free once the request ends — which the spec then asserts. + */ +component extends="Controller" { + + function abortDefault() { + $probe(name = "probe_abort_default", tx = false, ending = "abort"); + } + + function redirectDefault() { + $probe(name = "probe_redirect_default", tx = false, ending = "redirect"); + } + + function abortTransaction() { + $probe(name = "probe_abort_tx", tx = true, ending = "abort"); + } + + function redirectTransaction() { + $probe(name = "probe_redirect_tx", tx = true, ending = "redirect"); + } + + private function $probe(required string name, required boolean tx, required string ending) { + var probeName = arguments.name; + var probeEnding = arguments.ending; + // A second-connection adapter on the model's own datasource, built BEFORE the lock so the + // callback closure only references captured locals (invariant 3). $isAdvisoryLockHeld() is a + // server-wide query, so from this separate connection a true result proves the lock really is + // held during the callback. + var classData = model("author").$classData(); + var folder = Left(get("adapterName"), Len(get("adapterName")) - 5); + var checker = CreateObject("component", "wheels.databaseAdapters." & folder & "." & get("adapterName")).$init( + dataSource = classData.dataSource, + username = classData.username, + password = classData.password + ); + + model("author").withAdvisoryLock(name = probeName, transaction = arguments.tx, callback = function() { + server["wheelsAdvisoryProbe_" & probeName] = checker.$isAdvisoryLockHeld(name = probeName); + if (probeEnding == "redirect") { + redirectTo(url = "/"); + } else { + abort; + } + }); + } +} diff --git a/vendor/wheels/tests/routes.cfm b/vendor/wheels/tests/routes.cfm index 41f88cbb87..9d06f7d3ed 100644 --- a/vendor/wheels/tests/routes.cfm +++ b/vendor/wheels/tests/routes.cfm @@ -17,6 +17,16 @@ mapper() .post(name="browserTestLogout", pattern="/logout", to="BrowserTestSessions##destroy") .get(name="browserTestLoginAs", pattern="/login-as", to="BrowserTestLogin##create") .end() + // Advisory-lock release-on-abort fixtures (#4219) — a lock taken by + // withAdvisoryLock() whose callback ends the request via abort/redirectTo + // must still be released (the release runs in a finally). Driven by + // wheels.tests.specs.model.advisoryLockAbortReleaseSpec. Must precede .wildcard(). + .scope(path="/_advisorylock") + .get(name="advisoryLockAbortDefault", pattern="/abort-default", to="AdvisoryLockProbe##abortDefault") + .get(name="advisoryLockRedirectDefault", pattern="/redirect-default", to="AdvisoryLockProbe##redirectDefault") + .get(name="advisoryLockAbortTx", pattern="/abort-tx", to="AdvisoryLockProbe##abortTransaction") + .get(name="advisoryLockRedirectTx", pattern="/redirect-tx", to="AdvisoryLockProbe##redirectTransaction") + .end() .wildcard() .get(name="wheelstestbox", pattern="wheels/core/tests", to="wheels##public##tests") .get(name="sampleLinkToTest", pattern="sample/linktotest", to="sample##linktotest") diff --git a/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc new file mode 100644 index 0000000000..7a8c157b85 --- /dev/null +++ b/vendor/wheels/tests/specs/model/advisoryLockAbortReleaseSpec.cfc @@ -0,0 +1,159 @@ +/** + * #4219: a withAdvisoryLock() callback that ends the request from inside the lock — with a real + * `abort` or a real `redirectTo()` (which ends in a cflocation) — must still release the database + * lock. The release runs in a `finally`, which executes on those exit paths (measured on Lucee, + * Adobe and BoxLang; only a `catch` is skipped by abort). This spec pins that: it drives + * request-ending actions over HTTP via $testClient and then, from its own separate connection, + * asserts the named lock is no longer held anywhere. A release that regressed out of the `finally` + * would leave the lock on the aborting request's pooled connection and red this spec. + * + * Covered: the default (session-scoped) path and the transaction = true path, each ending via abort + * and via redirectTo. Advisory locks only apply to PostgreSQL, MySQL and SQL Server, so other + * databases skip. The lock-held check uses the adapter's own $isAdvisoryLockHeld (the same + * canonical free-check the release verification uses), on a fresh adapter/connection. + * + * The transaction = true abort case runs on every advisory-lock database, but means something + * different per lock scope. On SESSION-scoped engines ($transactionalAdvisoryLockIsSessionScoped() + * == true: MySQL, SQL Server) it proves the session lock is released on abort. On PostgreSQL the + * transactional lock is pg_advisory_xact_lock, which the database auto-releases when the transaction + * ends — so it can't leak, but a request that never ends keeps it held; there the case becomes a + * hang detector, driven with a bounded $testClient timeout so a stuck request fails THIS case rather + * than timing out the whole engine leg. The transaction = true redirect case skips on PostgreSQL + * (redirect/cflocation completes the transaction normally, so there is nothing new to prove there). + * + * Fixtures: vendor/wheels/tests/_assets/controllers/AdvisoryLockProbe.cfc + the /_advisorylock + * routes in vendor/wheels/tests/routes.cfm. + */ +component extends="wheels.WheelsTest" { + + // A separate adapter instance on the test datasource, so the held-check runs on its own + // connection rather than the model's (mirrors advisoryLockTransactionalSpec.freshAdapter()). + function freshAdapter() { + var classData = variables.g.model("author").$classData(); + var folder = Left(variables.adapterName, Len(variables.adapterName) - 5); + return CreateObject("component", "wheels.databaseAdapters.#folder#.#variables.adapterName#").$init( + dataSource = classData.dataSource, + username = classData.username, + password = classData.password + ); + } + + // Best-effort: if a scenario failed and left a probe lock held, acquiring+releasing it through the + // public API clears it when it is free (a no-op) and times out harmlessly when a leaked session + // still holds it on a pooled connection (only that session could release it). Either way a failure + // cannot silently hand a held lock to a later spec. Each name is cleaned independently. + function releaseProbeLocks() { + var noop = function() { + return ""; + }; + for (var probe in ListToArray(variables.probeNames)) { + try { + variables.g.model("author").withAdvisoryLock(name = probe, timeout = 1, callback = noop); + } catch (any e) { + // lock still held by a leaked session, or the DB does not support it — nothing to do. + } + } + } + + function run() { + // Set at registration time (not beforeAll): the describe callback below reads these while + // the suite is being built, before any lifecycle method runs. + variables.g = application.wo; + variables.adapterName = variables.g.get("adapterName"); + // Databases whose adapter takes real advisory locks (mirrors advisoryLockExclusionSpec / + // advisoryLockTransactionalSpec). Everything else has no lock to leak, so it skips. + variables.applies = ListFindNoCase("PostgreSQLModel,MySQLModel,MicrosoftSQLServerModel", variables.adapterName) > 0; + variables.probeNames = "probe_abort_default,probe_redirect_default,probe_abort_tx,probe_redirect_tx"; + + describe("withAdvisoryLock releases the lock when the callback ends the request (4219)", () => { + + if (!variables.applies) { + it("skips on databases without advisory-lock support", () => { + skip("Advisory locks apply to PostgreSQL, MySQL and SQL Server only (adapter: " & variables.adapterName & ")."); + }); + return; + } + + afterEach(() => { + for (var probe in ListToArray(variables.probeNames)) { + StructDelete(server, "wheelsAdvisoryProbe_" & probe); + } + releaseProbeLocks(); + }); + + it("default path: frees the lock after the callback aborts", () => { + var tc = $testClient(); + StructDelete(server, "wheelsAdvisoryProbe_probe_abort_default"); + tc.get("/_advisorylock/abort-default"); + expect(tc.statusCode()).toBe(200, "the abort probe request did not complete with 200"); + // positive control: the callback ran inside the lock, and a second connection saw it held + expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_abort_default")).toBeTrue( + "the probe callback never ran — the request did not reach withAdvisoryLock" + ); + expect(server["wheelsAdvisoryProbe_probe_abort_default"]).toBeTrue( + "the lock was not observably held during the callback — nothing to prove released" + ); + expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_abort_default")).toBeFalse( + "a default-path lock stayed held after the callback aborted — the release did not run on abort" + ); + }); + + it("default path: frees the lock after the callback redirects", () => { + var tc = $testClient(); + StructDelete(server, "wheelsAdvisoryProbe_probe_redirect_default"); + tc.get("/_advisorylock/redirect-default"); + tc.assertRedirect(); + expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_redirect_default")).toBeTrue( + "the probe callback never ran — the request did not reach withAdvisoryLock" + ); + expect(server["wheelsAdvisoryProbe_probe_redirect_default"]).toBeTrue( + "the lock was not observably held during the callback — nothing to prove released" + ); + expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_redirect_default")).toBeFalse( + "a default-path lock stayed held after the callback redirected — the release did not run on cflocation" + ); + }); + + it("transaction = true: frees the lock after the callback aborts", () => { + var tc = $testClient(); + StructDelete(server, "wheelsAdvisoryProbe_probe_abort_tx"); + // Bounded timeout: on a transaction-scoped-lock engine (PostgreSQL) the lock is held for + // as long as the transaction is open, so a request that never returns keeps the lock held. + // A short timeout makes such a hang fail THIS assertion (statusCode != 200) instead of + // timing out the whole engine leg. Session-lock engines complete in milliseconds, so the + // bound never bites them. + tc.get(path = "/_advisorylock/abort-tx", timeout = 20); + expect(tc.statusCode()).toBe(200, "the abort probe request did not return 200 within the 20s bound (a transaction-scoped-lock engine that hangs here would keep the lock held)"); + expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_abort_tx")).toBeTrue( + "the probe callback never ran — the request did not reach withAdvisoryLock" + ); + expect(server["wheelsAdvisoryProbe_probe_abort_tx"]).toBeTrue( + "the lock was not observably held during the callback — nothing to prove released" + ); + expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_abort_tx")).toBeFalse( + "a transaction-path lock stayed held after the callback aborted — the release did not run on abort" + ); + }); + + it("transaction = true: frees the lock after the callback redirects", () => { + if (!freshAdapter().$transactionalAdvisoryLockIsSessionScoped()) { + skip("transaction=true uses a transaction-scoped advisory lock on " & variables.adapterName & " (PostgreSQL's pg_advisory_xact_lock), auto-released by the database at transaction end — there is no session-held lock to leak when the callback redirects."); + return; + } + var tc = $testClient(); + StructDelete(server, "wheelsAdvisoryProbe_probe_redirect_tx"); + tc.get("/_advisorylock/redirect-tx"); + tc.assertRedirect(); + expect(StructKeyExists(server, "wheelsAdvisoryProbe_probe_redirect_tx")).toBeTrue( + "the probe callback never ran — the request did not reach withAdvisoryLock" + ); + expect(server["wheelsAdvisoryProbe_probe_redirect_tx"]).toBeTrue( + "the lock was not observably held during the callback — nothing to prove released" + ); + expect(freshAdapter().$isAdvisoryLockHeld(name = "probe_redirect_tx")).toBeFalse( + "a transaction-path lock stayed held after the callback redirected — the release did not run on cflocation" + ); + }); + }); + } +} diff --git a/vendor/wheels/wheelstest/TestClient.cfc b/vendor/wheels/wheelstest/TestClient.cfc index 92213af5d8..fc2777338c 100644 --- a/vendor/wheels/wheelstest/TestClient.cfc +++ b/vendor/wheels/wheelstest/TestClient.cfc @@ -287,13 +287,16 @@ component { * @path URL path (appended to baseUrl) * @params Query string parameters as a struct * @headers Additional headers for this request + * @timeout cfhttp timeout in seconds (default 30). Lower it to bound a request that may hang so + * the caller's assertion fails rather than the whole test leg timing out. */ public TestClient function get( required string path, struct params = {}, - struct headers = {} + struct headers = {}, + numeric timeout = 30 ) { - $makeRequest(method = "GET", path = arguments.path, params = arguments.params, headers = arguments.headers); + $makeRequest(method = "GET", path = arguments.path, params = arguments.params, headers = arguments.headers, timeout = arguments.timeout); return this; } @@ -733,7 +736,8 @@ component { required string path, struct params = {}, struct body = {}, - struct headers = {} + struct headers = {}, + numeric timeout = 30 ) { $requireLeadingSlash(arguments.path); @@ -755,7 +759,7 @@ component { var result = {}; - cfhttp(url = fullUrl, method = arguments.method, timeout = "30", result = "result", redirect = false) { + cfhttp(url = fullUrl, method = arguments.method, timeout = arguments.timeout, result = "result", redirect = false) { // Add merged headers for (var hName in mergedHeaders) { cfhttpparam(type = "header", name = hName, value = mergedHeaders[hName]);