-
Notifications
You must be signed in to change notification settings - Fork 15
[Fix] streaming: do not reset timeout for each chunk emit #191
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
base: main
Are you sure you want to change the base?
Conversation
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Outdated
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Outdated
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
| stream.Emit("Eleventh message"); | ||
| stream.Emit("Twelfth message"); | ||
|
|
||
| await Task.Delay(70); // Wait for initial flush |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as my python feedback, instead of using a delay you should test this via a stubbed HttpClient so you can programmatically throw different status codes to test the streamer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing unit tests for AspNetCorePlugin.Stream will depend on FakeTimeProvider and required internal timing hooks (LastFlushTask) to be deterministic. Maintaining these tests would require changes to production code that are not justified by their limited value and the complexity introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a streaming timeout issue where the timeout was being reset on each chunk emission, preventing timely flushing of streaming data. The fix changes the behavior to flush immediately when no timeout is pending, rather than continuously resetting a timer.
Key changes:
- Modified
Emitmethods to trigger immediate flush when no timeout is pending, instead of resetting timer on each emit - Changed lock behavior from blocking to non-blocking using
WaitAsync(0)to avoid multiple concurrent flushes - Added timeout handling in
Close()method with newWaitForIdAndQueueAsync()helper to prevent indefinite waiting - Increased global retry delay from 200ms to 500ms to avoid API quota issues
- Removed all existing streaming tests
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs | Core streaming implementation changes: modified emit logic to not reset timeout, added timeout handling in Close(), changed to non-blocking lock acquisition, and added comprehensive documentation |
| Libraries/Microsoft.Teams.Common/Extensions/TaskExtensions.cs | Increased default retry delay from 200ms to 500ms globally to prevent API quota exceeded errors |
| Tests/Microsoft.Teams.Plugins.AspNetCore.Tests/AspNetCorePluginStreamTests.cs | Removed all streaming tests (4 test methods covering flush timing, timer resets, timeout handling, and typing activities) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Outdated
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
…etCore/AspNetCorePlugin.Stream.cs Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue : microsoft/teams.ts#374
Main change::
EDIT
WaitForIdAndQueueAsync: On close, donot wait forever for id to be set or queue to be empty. Leads to hangingstreaming_dotnet_fix.mp4
Devtools: Streaming always starts before the full response is received (check with logs, see attached video with the green coloured log).
Teams: Even though the first chunk is emitted immediately, (sometimes) by the time the stream starts on Teams, the full response is ready. In the third msg ?, we can see the stream starts a slightly before the full response is logged.