Repository navigation
[BUGFIX] Reset alreadyDisplayed news list between requests - #2859
Open
contemas-tschmidt wants to merge 2 commits into
Open
contemas-tschmidt wants to merge 2 commits into
contemas-tschmidt wants to merge 2 commits into
Conversation
ExcludeDisplayedNewsViewHelper and NewsRepository::findDemanded() track already-rendered news UIDs in $GLOBALS['EXT']['news']['alreadyDisplayed'] to support the excludeAlreadyDisplayedNews demand. This array is a plain PHP superglobal, not request-scoped state, and nothing in the extension ever clears it. Under PHP-FPM (or any SAPI that reuses one PHP process across several HTTP requests) the array keeps every UID added by every request handled by that worker, for the worker's entire lifetime. The resulting NOT IN (...) condition in NewsRepository grows without bound, and on a site with a few thousand news records this reliably exhausts memory_limit with a "Allowed memory size exhausted" fatal error after enough requests on the same worker - with a stack trace pointing at whatever allocation happened to be the last one to fail, which has nothing to do with the actual root cause. Add a frontend middleware that resets the array at the start of every request. This keeps the intended per-page behaviour (no duplicate news within a single page) while removing the cross-request growth. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
The CI run showed two unrelated failures:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ExcludeDisplayedNewsViewHelperandNewsRepository::findDemanded()track already-rendered news UIDs in$GLOBALS['EXT']['news']['alreadyDisplayed']to support theexcludeAlreadyDisplayedNewsdemand. This array is a plain PHP superglobal, not request-scoped state, and nothing in the extension ever clears it.Under PHP-FPM (or any SAPI that reuses one PHP process across several HTTP requests) the array keeps every UID added by every request handled by that worker, for the worker's entire lifetime. The resulting
NOT IN (...)condition inNewsRepositorygrows without bound, and on a site with a few thousand news records this reliably exhaustsmemory_limitwith an "Allowed memory size exhausted" fatal error after enough requests land on the same worker - with a stack trace pointing at whatever allocation happened to be the last one to fail, which has nothing to do with the actual root cause. This made it very hard to track down in practice.I couldn't find an existing issue describing this specific failure mode, so I'm opening this PR directly together with the fix.
Fix
Add a frontend middleware that resets the array at the start of every request. This keeps the intended per-page behaviour (no duplicate news within a single page) while removing the cross-request growth under FPM.
Testing
Reproduced the crash locally and in a staging environment with
excludeAlreadyDisplayedNewsenabled on a news list, confirmed repeated requests against the same long-lived worker process eventually crash without this fix, and confirmed the fix resolves it (many consecutive requests succeed cleanly with the middleware in place).