Skip to content

Fix database deadlock exception in AssetService::getTypoScript #1945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

adamkoppede
Copy link
Contributor

If the database caching backend is used for the "vhs_main" cache, database deadlocks occur if there are many concurrent requests to a page with uncached content. A full reproduction example is available on GitHub: https://github.com/adamkoppede/reproduce-deadlock-with-fluidtypo3-vhs-7-1

The deadlock occurs in the EXT:core Typo3DatabaseBackend::set() method. I created an issue in the upstream TYPO3 bug tracker: https://forge.typo3.org/issues/106593

Currently Typo3DatabaseBackend::set() is called four times per HTTP request. The first commit in this change set extends the static::$settingsCache so it contains plugin.tx_vhs fully, instead of just plugin.tx_vhs.settings; this reduces the Typo3DatabaseBackend::set() calls down to one per request. The second commit removes the Typo3DatabaseBackend::set() call for pages with uncached content, as they cannot be served from cache.

@adamkoppede adamkoppede force-pushed the bugfix/database-deadlock-exception-in-AssetService branch from c1a42f5 to 57203d8 Compare April 21, 2025 21:45
@adamkoppede

This comment was marked as outdated.

With TYPO3 v13, AssetService calls FrontendInterface::set() four times
in AssetService::getTypoScript() for a single AssetService::buildAll()
invocation. The cache usage was introduced in e84af96 ("[BUGFIX] Handle
new TS-not-set limitation on v13 (FluidTYPO3#1928)"), as TYPO3 v13 no longer fully
sets up TypoScript when serving fully-cached pages.

The four callers of AssetService::getTypoScript() are:

- AssetService::getSettings()
- AssetService::writeCachedMergedFileAndReturnTag()
- AssetService::getFileIntegrity()
- AssetService::generateTagForAssetType()

Reducing the number of FrontendInterface::set() calls reduces the amount
of database deadlock errors that occur if the Typo3DatabaseBackend is
used for the "vhs_main" cache and many concurrent requests hit the same
TYPO3 page that contains at least one uncached content element. But it
doesn't solve them completely; e.q. They still occur but less frequent.
This issue is tracked in the upstream TYPO3 bug tracker as #106593 [1].
A reproduction example is available on GitHub [2].

[1]: https://forge.typo3.org/issues/106593
[2]: https://github.com/adamkoppede/reproduce-deadlock-with-fluidtypo3-vhs-7-1
@NamelessCoder NamelessCoder force-pushed the bugfix/database-deadlock-exception-in-AssetService branch from 57203d8 to 06127d6 Compare May 7, 2025 10:54
Copy link
Member

@NamelessCoder NamelessCoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general this very much looks like a job for a lock (semaphore or file ideally) rather than a range of workarounds. Using TYPO3's locking mechanism also allows users to configure how this locking happens (good for distributed systems).

And I guess it would be OK to make the thread wait for a lock release (maybe adding a maximum wait time).

There may also be a bug or unforeseen circumstance in how the AssetService uses the internal cache and writes to the cache, e.g. a missing condition to prevent cache writing when cache has already been written (which should not need new has-written tracking, see review comments).

But a lot of the code seems unnecessary or I don't understand the need for it. For example, keeping track of multiple page IDs, tracking whether the end result is cacheable, and storing those things in static variables.

Since change e84af96 ("[BUGFIX] Handle new TS-not-set limitation on v13
(FluidTYPO3#1928)") the "plugin.tx_vhs" section of the TypoScript setup is written
into a cache. This is necessary as TYPO3 v13
PrepareTypoScriptFrontendRendering [1] no longer performs the full
TypoScript setup when serving a fully-cached page. An "Setup array has
not been initialized" error is thrown when trying to access the
uninitialized TypoScript setup.

For pages with uncached content, FrontendInterface::set() is called on
each request. This leads to database deadlock errors if the
Typo3DatabaseBackend is used for the "vhs_main" cache and many
concurrent requests hit the same TYPO3 page. This issues is tracked in
upstream TYPO3 bug tracker as #106593 [2]. A fully reproduction example
is available on GitHub [3].

We don't need to store the TypoScript of those pages with uncached
content in the cache, because they cannot be served from cache and thus
force full TypoScript setup even when answering subsequent requests.

[1]: https://github.com/TYPO3/typo3/blob/v13.4.9/typo3/sysext/frontend/Classes/Middleware/PrepareTypoScriptFrontendRendering.php#L146
[2]: https://forge.typo3.org/issues/106593
[3]: https://github.com/adamkoppede/reproduce-deadlock-with-fluidtypo3-vhs-7-1
@adamkoppede adamkoppede force-pushed the bugfix/database-deadlock-exception-in-AssetService branch from 06127d6 to afe6cb8 Compare May 7, 2025 12:20
@adamkoppede
Copy link
Contributor Author

adamkoppede commented May 7, 2025

I think in general this very much looks like a job for a lock

A lock would definitely fix the Deadlock Exceptions. But it would also slow down the more common case, where there are no concurrent requests to the same page. I was trying to avoid that, especially since 7.1.x is already more expensive than 7.0.x on older TYPO3 versions.

tracking whether the end result is cacheable

This specifically targets our customers use-case where we observed the issue first: a single page with USER_INT that contains thousands of "virtual subpages"1. Due to the USER_INT, TypoScript is always loaded when answering the requests; so we don't need to persist the TypoScript.

storing those things in static variables.

I agree. That part could be improved. But such a refactor would touch more methods of that class.

Footnotes

  1. Think of it as EXT:news plugin in the List + Detail page configuration

@adamkoppede adamkoppede requested a review from NamelessCoder May 8, 2025 11:44
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.

2 participants