fix: prevent GraphQLCollector from accumulating batches across requests - #1250
Open
w0lan wants to merge 1 commit into
Open
fix: prevent GraphQLCollector from accumulating batches across requests#1250w0lan wants to merge 1 commit into
w0lan wants to merge 1 commit into
Conversation
GraphQLCollector appends one entry per executed GraphQL operation to $batches, each holding a VarDumper clone of the full response, and nothing cleared it between requests: reset() cleared only $data, and the collector carried no kernel.reset tag, so where it is registered while Symfony's profiler service is not, reset() was never called at all. reset() now clears $batches as well, and profiler.yaml tags the collector kernel.reset so ServicesResetter reaches it directly, the same way Executor and TypeResolver already do (overblog#1203). Unlike the compile-time schema state involved in overblog#1242, $batches is per-request runtime state, so clearing it loses nothing. The base DataCollector serializes only $data into the persisted profile, so the profiler panel is unaffected.
Author
|
The red jobs are the pre-existing |
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.
masterunchanged — see Testing)Summary
GraphQLCollectorappends one entry per executed GraphQL operation to$batches, and eachentry holds a
VarDumperclone of the full response. Two independent things keep that arrayfrom being cleared between requests in a long-running process:
reset()clears$databut leaves$batchesuntouched;kernel.resettag, so in an application where the collector isregistered while Symfony's
profilerservice is not,reset()is never called at all —the
profilerservice is the only thing that forwardsreset()to data collectors, and itonly exists when
framework.profileris enabled.This PR fixes both:
reset()now clears$batches, andprofiler.yamltags the collectorkernel.resetso thatServicesResetterreaches it directly, exactly likeExecutorandTypeResolveralready do since #1203. (Unlike the schema state involved in #1242,$batchesis purely per-request runtime state, so clearing it cannot lose anything — details below.)
Where this actually bites
(Symfony line references below are from 7.3.)
Under php-fpm the request teardown frees the whole userland heap after every request, so the
array never survives — regardless of process reuse. (One nuance:
FrameworkBundle'sHttpCachecallsKernel::boot()per forwarded ESI fragment,HttpCache.php:65, whichtriggers the resetter mid-response.) Under a
long-running runtime (RoadRunner, Swoole, FrankenPHP worker mode) the process serves thousands
of requests,
Kernel::boot()runs the resetter between them (Kernel.php:106-109), and everyrequest leaves another response clone behind. Concretely:
framework.profiler.enabled: true+collect: false(on-demand profiling, common onstaging/production), or
only_exceptions: true, or aRequestMatcher:Profiler::collect()early-returns (
Profiler.php:132-134), soGraphQLCollector::collect()never runs and$datastays empty — butonPostExecutor()keeps firing, because it is an ordinary eventlistener.
Profiler::reset()is called here, and today it clears nothing. Fixed by change 1.symfony/runtimeworker mode with the profiler on): same thing,bounded only by how long the worker lives.
overblog_graphql.profiler.enabled: truewhile
framework.profileris off, and, before fix: make profiler configurable and disable it by default in non-debug mode #1243, any non-debug application, sinceprofiler.yamlwas loaded unconditionally. Here there is noprofilerservice, so nothingcalls
reset()on the collector at all, and change 1 alone would be a no-op. Fixed by change 2.The change
public function reset(): void { $this->data = []; + $this->batches = []; }- { name: kernel.event_listener, event: graphql.post_executor, method: onPostExecutor } + - { name: kernel.reset, method: reset }services_resetteris defined unconditionally by FrameworkBundle(
Resources/config/services.php:182) andResettableServicePasswires everykernel.resetservice into it, using an
IGNORE_ON_UNINITIALIZED_REFERENCEreference — so a collector thatwas never instantiated during the request is skipped, and one that was gets cleared. When the
Symfony profiler is enabled,
reset()simply runs twice (once throughProfiler::reset(),once directly); it is idempotent.
I deliberately did not touch
collect()oronPostExecutor(). Capping the number of retainedbatches, or storing the response size instead of a full
cloneVar($result), would also boundthe worst case within a single request, but that changes what the profiler panel shows and
belongs in a separate discussion.
Why this cannot blind the profiler panel
Two mechanisms, neither of which depends on event ordering:
Profiler::collect()storesclone $collectorin theProfile(Profiler.php:157-161);GraphQLCollectorhas no__clone(), so the arrays are copied by value and a laterreset()on the live service cannot reach them.
DataCollectorserializes only$data, so the persisted profile never contained$batchesin the first place. The panel reads whatcollect()copied into$data['batches'].Batched requests are also safe:
GraphController::processBatchQuery()executes the whole batchinside one controller call, so all operations of one HTTP request land in
$batchesbeforekernel.response.Consistency with the rest of the ecosystem
Every Symfony collector that accumulates working state during a request clears it in
reset()—SerializerDataCollectoris the closest structural twin (it also fills a private array fromoutside
collect();SerializerDataCollector.php:40-45), andRequestDataCollector,RouterDataCollector,DumpDataCollector,DoctrineDataCollectorand others do the same fortheir own buffers.
GraphQLCollectoris the outlier; this brings it in line.This is not a repeat of #1242
Executor::$schemaswas populated once at container compile time, which is why resetting itlost data permanently.
$batchesis filled purely at runtime, per request, by thegraphql.post_executorlistener — there is no compile-time state to lose.Relation to #1243
#1243 already identified this accumulation ("All this data was accumulated into
$this->batchesand never consumed") and fixed the exposure by not registering the collectoroutside debug mode. That was the right call and it covers the common production setup. This PR
fixes the accumulator itself, for every configuration where the collector is registered on
purpose.
How big the accumulator is
Measured on a Symfony 7 GraphQL API running v1.7.0 with the collector registered and never
reset (case 3 above): heap dumps under RoadRunner after 901 requests showed
$batchesholding70.18 MB out of 77.12 MB (91%) of all traced worker retention; on FrankenPHP worker mode a
search query with a ~66.8 KB response grew the worker heap by ~199 KB per request with no
plateau, and
VarDumper\Cloner\Datagrew by exactly two objects per executed operation,with nothing freed during the run.
Those numbers quantify the accumulator, not this patch. The
reset()one-liner alone measuredno change in that setup (without the tag, nothing called
reset()), and our own productionproblem was solved by not registering the collector — the same approach as #1243. With the
complete patch, in the same setup with the collector registered, worker heap growth went from
205.6 KB/req to flat after warm-up, and a heap dump after 301 requests showed
$batchesholding a single entry instead of 301.
Testing
GraphQLCollectorTest::testResetClearsBatches()— unit level: collect a batch,reset(),and verify through the public API that a following
collect()reports no leftovers and thata batch collected afterwards is the only one reported.
Functional\DataCollector\GraphQLCollectorResetTest— wiring level: boot the testapplication (which has
framework.profiler.enabled: false, so the collector is registeredwithout Symfony's profiler), perform a GraphQL request, then call
services_resetter->reset()the way
Kernel::boot()does between requests, and assert the collector reports nothing.Both fail on
master. The functional one also fails with only thereset()change applied, soit covers the tag as well.
Verified on PHP 8.1 (Symfony 6.4 resolution) and PHP 8.4/8.5 (Symfony 8.1 resolution).
Symfony 5.4 and 7.x were not run here; everything the functional test relies on is identical
in those branches (
ResettableServicePass, the unconditional publicservices_resetter,test.service_container). Full suite: 714 tests (712 onmasterplus the two added here); the5 failures and 7 errors reproduce identically on an unmodified
mastercheckout (a newerwebonyx/graphql-phpresolving without a committed lock file, plus a PHP 8.5 deprecation froma transitive dependency) — the patch adds two tests and no new failures.
check-csis clean;static-analysisadds no new findings beyond more instances of a pre-existing symbol-discoveryartifact this environment already reports for the existing tests.
Thanks for the bundle, and for the recent work on worker-mode support.