Conversation
Summary of ChangesHello @AustinBenoit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Firebase Functions client library by migrating its core logic from C++ SWIG bindings to a pure C# implementation. This change streamlines the codebase, reduces external dependencies, and provides more direct control over HTTP request handling and data serialization within the C# environment. It also introduces new internal helper classes for HTTP operations and error parsing, ensuring a robust and consistent experience for developers while maintaining backward compatibility. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The rewrite of Firebase Functions in C# significantly improves maintainability by removing the C++ dependency. The implementation correctly handles the core protocol and serialization. However, there are technical concerns regarding resource management and code quality. Specifically, HttpClient should be shared at the service level to avoid socket exhaustion, and the reflection-based event subscription in FirebaseFunctions is fragile and potentially leaks memory. Additionally, several 'TODO' comments should be resolved, and CancellationToken support should be added to the public API to align with modern .NET practices.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and welcome rewrite of the Firebase Functions SDK, moving it from a C++/SWIG implementation to a pure C# implementation. This removes a native dependency and simplifies the build process, as reflected in the CMakeLists.txt changes. The refactoring also includes moving shared utilities like HttpHelpers and FirebaseInterops to a common app/src/internal location, which is a good improvement for code sharing across Firebase products.
My review has identified a few critical issues:
- A memory leak in
FirebaseFunctions.csdue to incorrect event handler disposal. - A syntax error in
FunctionsException.csthat will break compilation.
I've also included some suggestions to improve maintainability regarding the use of reflection and to clean up leftover debug code. Overall, this is a great direction for the SDK.
AustinBenoit
force-pushed
the
functionsrewrite
branch
4 times, most recently
from
0cb7d97 to
9ed1eb4
Compare
This is the first phase where I get things building again.
AustinBenoit
force-pushed
the
functionsrewrite
branch
from
9ed1eb4 to
fa0e5a3
Compare
|
/gemini review |
AustinBenoit
added
the
tests-requested: quick
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable rewrite of the Firebase Functions implementation, moving from a C++/SWIG wrapper to a pure C# implementation using HttpClient. This greatly improves maintainability and portability. The new implementation of serialization and error parsing seems correct. However, I've found a few critical issues that need to be addressed: a memory leak due to incorrect event handler removal, and a race condition in instance management. I've also pointed out a couple of minor issues like a leftover debug log and some unused using statements.
github-actions
bot
added
tests: in-progress
❌ Integration test FAILEDRequested by @firebase-workflow-trigger[bot] on commit 7148241
|
firebase-workflow-trigger
bot
removed
the
tests: in-progress
|
/gemini review |
AustinBenoit
added
tests-requested: quick
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the Firebase Functions implementation in C#, moving away from the C++ SWIG bindings to a pure C# approach. This change involves a complete rewrite of the FirebaseFunctions and HttpsCallableReference classes, along with the introduction of new internal serialization and error parsing utilities. The build system has also been updated to reflect this change, removing native library dependencies and packaging C# source files directly. Additionally, a configurable authentication token prefix has been introduced for greater flexibility in handling Firebase tokens.
Overall, the changes represent a substantial improvement in maintainability and potentially performance by removing the C++ interop layer. However, there are a few areas that could be improved to enhance robustness and clarity.
| private void Dispose() { | ||
| System.GC.SuppressFinalize(this); | ||
| lock (functionsByInstanceKey) { | ||
| if (functionsInternal == null) return; | ||
| functionsByInstanceKey.Remove(instanceKey); | ||
|
|
||
| functionsInternal.Dispose(); | ||
| functionsInternal = null; | ||
|
|
||
| firebaseApp.AppDisposed -= OnAppDisposed; | ||
| _instances.TryRemove(_instanceKey, out _); | ||
| if (_appDisposedEvent != null && _onAppDisposedHandler != null) { | ||
| var removeMethod = _appDisposedEvent.GetRemoveMethod(true); | ||
| removeMethod?.Invoke(_firebaseApp, new object[] { _onAppDisposedHandler }); | ||
| } | ||
| _httpClient.Dispose(); |
There was a problem hiding this comment.
The Dispose method does not fully implement the IDisposable pattern. If Dispose() is called multiple times, it could lead to an ObjectDisposedException when trying to dispose _httpClient again. Consider adding a _disposed flag to ensure resources are only released once.
private bool _disposed = false;
private void Dispose() {
if (_disposed) return;
System.GC.SuppressFinalize(this);
_instances.TryRemove(_instanceKey, out _);
if (_appDisposedEvent != null && _onAppDisposedHandler != null) {
var removeMethod = _appDisposedEvent.GetRemoveMethod(true);
removeMethod?.Invoke(_firebaseApp, new object[] { _onAppDisposedHandler });
}
_httpClient.Dispose();
_disposed = true;
}| _instanceKey = InstanceKey(app, region); | ||
|
|
||
| // Default timeout is 70 seconds matching native SDKs. | ||
| _httpClient = new HttpClient(); |
There was a problem hiding this comment.
The value 70 for the HttpClient timeout is a magic number. It would be more maintainable and readable to define this as a named constant, allowing for easier modification and understanding of its purpose.
private const int DefaultHttpClientTimeoutSeconds = 70;
_httpClient = new HttpClient();
_httpClient.Timeout = TimeSpan.FromSeconds(DefaultHttpClientTimeoutSeconds);| #if FIREBASE_LOG_REST_CALLS | ||
| UnityEngine.Debug.Log("Request:\n" + request.Content); | ||
| #endif | ||
| // TODO pipe through cancellation tokens |
There was a problem hiding this comment.
The TODO comment indicates that cancellation tokens should be piped through. Implementing this would improve the responsiveness and resource management of asynchronous operations, especially in scenarios where long-running function calls might need to be aborted.
github-actions
bot
added
tests: in-progress
firebase-workflow-trigger
bot
removed
the
tests: in-progress
1 participant
Description
Functions rewrite in C# with the goal of making this a non breaking change.
There is a couple of quirks with the reimplementation. In the previous iteration we used the C++ Variant object class that would do some type conversion. Now I have attempt to rewrite these type conversion in C#. This is just a bit odd as technically we are sending all ints as longs to the firebase functions. This is consistent with the previous implementation just a bit odd.
The second quirk is the how the bearer token is expected to be handled between AI logic and Functions. So we need to pipe through this specific name in each product.
Testing
Ran both the functions and AI test locally in unity.
Type of Change
Place an
xthe applicable box: