[Platform] Make DeferredResult and RawHttpResult safe to dump()/dd() - #2501
alireza-aminzadeh wants to merge 2 commits into
Conversation
|
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
chr-hertel
left a comment
There was a problem hiding this comment.
Nice idea, thanks @alireza-aminzadeh! Haven't tested it yet, but I think for the DeferredResult it would be nice to keep more of the instance's state, e.g. error message, metadata, options
Every HTTP-based ModelClient wraps its injected client with
EventSourceHttpClient (needed to decode SSE streaming responses), so the
response held by RawHttpResult is always a Symfony HttpClient
AsyncResponse, itself wrapping a TraceableResponse whenever HTTP client
profiling is enabled, which is the default in a full-stack app's dev
environment. Neither class has a dedicated VarDumper caster, and both
keep their network stream alive until it is explicitly consumed.
dump()'ing or dd()'ing a DeferredResult right after
PlatformInterface::invoke(), before the result is ever read, therefore
reflects into that live, not-yet-consumed response instead of a safe
snapshot: closures bound to the response, its HTTP client and its
internal chunk-accounting state all get expanded, and poking at that
state from outside the client's own bookkeeping is exactly what the
AsyncResponse/TraceableResponse pair is not designed to tolerate.
Add __debugInfo() to RawHttpResult so it never exposes the live response
in a dump, replacing it with a short, static summary of its type instead
of letting the dumper walk into its internals. Add __debugInfo() to
DeferredResult so a dump also surfaces the conversion state ("pending",
"converted" or "failed") at a glance, without ever calling getResult()
itself, so inspecting a pending result cannot trigger a conversion as a
side effect.
Fix symfony#2280
…dResult::__debugInfo() 733d81e added __debugInfo() to DeferredResult, reporting only the conversion state ("pending", "converted" or "failed"). Per review, that is too little to be useful at a glance: knowing a result failed without seeing why still sends a developer back to calling getResult() (and therefore catching the exception) just to read the message. Extend __debugInfo() with three more entries, none of which touch $rawResult any more than $state already does: * options - the array DeferredResult was constructed with, e.g. the model options a call site passed to invoke(). It is a plain, readonly, always-initialized constructor argument. * metadata - $this->getMetadata()->all(), which starts out empty and, once conversion succeeds, only ever holds what a ResultConverter or a stream listener explicitly added to it (e.g. token usage), never the raw response itself. * error - $conversionFailure?->getMessage(), null unless the state is "failed". Reading the message does not walk the exception's (potentially deep) stack trace. Widen the @return docblock to an array shape accordingly, and add one test per new entry, covering the pending, converted and failed states.
733d81e to
d962cda
Compare
|
Thanks for the feedback! Pushed a commit that extends
Also widened the Rebased on the latest |
Every HTTP-based
ModelClientwraps its injected client withEventSourceHttpClient(needed to decode SSE streaming responses), so theresponse held by
RawHttpResultis always a Symfony HttpClientAsyncResponse, itself wrapping aTraceableResponsewhenever HTTP clientprofiling is enabled, which is the default in a full-stack app's dev
environment. Neither class has a dedicated VarDumper caster, and both keep
their network stream alive until it is explicitly consumed.
dump()'ing ordd()'ing aDeferredResultright afterPlatformInterface::invoke(), before the result is ever read, thereforereflects into that live, not-yet-consumed response instead of a safe
snapshot: closures bound to the response, its HTTP client and its internal
chunk-accounting state all get expanded, and poking at that state from
outside the client's own bookkeeping is exactly what the
AsyncResponse/TraceableResponsepair is not designed to tolerate.This PR adds
__debugInfo()toRawHttpResultso it never exposes the liveresponse in a dump, replacing it with a short, static summary of its type
instead of letting the dumper walk into its internals. It also adds
__debugInfo()toDeferredResultso a dump surfaces the conversion state(
pending,convertedorfailed) at a glance, without ever callinggetResult()itself, so inspecting a pending result cannot trigger aconversion as a side effect.
Tests assert that
__debugInfo()never calls any method on the wrappedresponse, that the
httpStreamproperty stays visible, and thatDeferredResult's reported state matches pending/converted/failed withouttriggering a conversion.
No
CHANGELOG.md/UPGRADE.mdchanges, since this is a bug fix only.