- Issue created by @svendecabooter
- Merge request !634#3526936: trigger PostStreamingResponseEvent when streamed chat message iterator gets destructed → (Open) created by svendecabooter
- 🇧🇪Belgium svendecabooter Gent
MR now triggers the event, when the StreamedChatMessageIterator object gets destructed.
- 🇧🇪Belgium svendecabooter Gent
Test
PromptJsonDecoderTest
seems to fail now, because\Drupal\ai\Event\PostStreamingResponseEvent::__construct
doesn't receive a request_thread_id string in its constructor.
Not sure if this value is NULL in this test by design, or by accident.
If by design, then the PostStreamingResponseEvent$request_thread_id
parameter should be allowed to be nullable? - 🇮🇳India sarvjeetsingh
Hi @svendecabooter,
The current destructor approach — it might be causing issues since it triggers for all iterator instances, including test objects.Would it make sense to consider moving the triggerEvent() call to the ChatOutput constructor instead? That way:
- It would only fire for actual streaming responses
- It wouldn’t interfere with test objects or other unintended casesLet me know what you think. Thanks!
- 🇧🇪Belgium svendecabooter Gent
@sarvjeetsingh:
I'm not sure if that would work - I'm still tipping my toes into how all of this works...
It would be something like this, in the ChatOutput constructor?:
if ($normalized instanceof StreamedChatMessageIteratorInterface) { $normalized->triggerEvent(); }
In my initial testing, that doesn't seem to work. I then get the same error as in the tests:
TypeError: Drupal\ai\Event\PostStreamingResponseEvent::__construct(): Argument #1 ($request_thread_id) must be of type string, null given
Not sure if that's something that also needs to be fixed, or if this means this approach is not workable...
- 🇮🇳India sarvjeetsingh
Thanks for testing the ChatOutput approach!
You're right about the NULL `request_thread_id` issue. The PostStreamingResponseEvent `$request_thread_id` parameter might need to be nullable - I think that makes sense.
We could make that constructor accept nullable string if that would help resolve this issue. That might be the right approach.
- 🇮🇳India annmarysruthy
I’ve made a few updates to fix the missing triggerEvent() call:
- Made $request_thread_id nullable in PostStreamingResponseEvent to avoid test failures.
- Added a $eventTriggered flag in StreamedChatMessageIterator to prevent duplicate events.
- Added null check in triggerEvent() and improved __destruct() for safety.
- Introduced finishStreaming() method so providers can trigger the event explicitly (recommended over relying on __destruct()).
Kindly review.