Skip to content

Functions: Add in function streaming#1478

Open
AustinBenoit wants to merge 5 commits into
mainfrom
functionsStreaming
Open

Functions: Add in function streaming#1478
AustinBenoit wants to merge 5 commits into
mainfrom
functionsStreaming

Conversation

@AustinBenoit

Copy link
Copy Markdown
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.

Functions: Add in function streaming


Testing

Describe how you've tested these changes.

Ran the test locally in unity editor and created a new integration test


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces Server-Sent Event (SSE) streaming support for callable functions in the Firebase Unity SDK, adding the StreamAsync API and a new StreamResponse class, along with corresponding integration tests and dependency updates. The review feedback highlights two important improvements in HttpsCallableReference.cs: first, the HttpResponseMessage returned by SendAsync should be disposed to prevent connection leaks when using HttpCompletionOption.ResponseHeadersRead; second, a null check should be added for response.Content before reading the stream to avoid potential null reference exceptions.

Comment thread functions/src/HttpsCallableReference.cs Outdated
#endif

// Use ResponseHeadersRead to avoid loading the whole stream at once.
var response = await _firebaseFunctions.HttpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The HttpResponseMessage returned by SendAsync is not disposed. When using HttpCompletionOption.ResponseHeadersRead, the underlying network connection remains active and open until the response is explicitly disposed. Failing to dispose it can lead to connection leaks, especially if the stream is not fully consumed or if an exception is thrown during iteration. Please use using var response to ensure proper resource cleanup.

      using var response = await _firebaseFunctions.HttpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread functions/src/HttpsCallableReference.cs
@AustinBenoit

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for Callable Functions Streaming in Firebase Functions via the new StreamAsync API, utilizing Server-Sent Events (SSE). It includes the implementation of StreamResponse to represent intermediate messages and final results, SSE stream parsing logic, and corresponding integration tests. Feedback on the changes suggests improving the streaming implementation by properly disposing of HttpRequestMessage, combining cancellation tokens, and ensuring responsive cancellation during stream reads.

Comment thread functions/src/HttpsCallableReference.cs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant