feat(functions): add Cloud Functions support for Windows desktop - #18471
feat(functions): add Cloud Functions support for Windows desktop#18471akshaynexus wants to merge 7 commits into
Conversation
Implements the CloudFunctionsHostApi pigeon interface (already generated into windows/messages.g.*) over the Firebase C++ SDK, mirroring the Linux implementation from firebase#18461: - httpsCallable / httpsCallableFromUrl calls with full Variant <-> EncodableValue conversion (including typed-data lists) - canonical error code mapping with code/message details for Dart's exception mapping - per-call timeout enforced by the plugin via a deadline thread, since the C++ SDK has no deadline API (responds deadline-exceeded; the SDK completion then only cleans up) - emulator origin support - streaming callables report 'unimplemented' (no C++ SDK support) Adds cloud_functions to the Windows e2e suite with skips limited to genuine C++ SDK gaps (streaming, error details payload) and a drain after the timeout test because the timed-out request cannot be cancelled.
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 the 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 counterproductive. 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. |
|
/gemini review |
![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
This pull request adds Windows platform support to the cloud_functions plugin using the Firebase C++ SDK, along with corresponding integration test updates. The code review highlights several critical issues in the C++ implementation, including a potential use-after-free vulnerability when converting blob data, thread-safety concerns from invoking the Pigeon callback on background threads, and redundant caching of raw Functions pointers that could lead to dangling pointers if Firebase apps are destroyed. Suggestions are provided to safely manage blob lifetimes, dispatch callbacks to the main thread, and track emulator configurations using a std::set.
The C++ SDK already caches instances per (app, region) inside Functions::GetInstance; a plugin-side raw-pointer cache could dangle if an app were deleted and recreated. Addresses review feedback.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds Windows support to the cloud_functions plugin using the Firebase C++ SDK, and updates the integration tests to handle desktop-specific limitations. The review feedback suggests two important improvements: first, avoiding manual heap allocation of HttpsCallableReference by capturing it by value in the completion lambda to leverage automatic lifetime management; second, ensuring that the platform channel result callbacks are dispatched back to the main thread to prevent thread-safety issues with the Flutter Windows engine.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Windows support for the cloud_functions package by implementing a C++ plugin backed by the Firebase C++ SDK, along with corresponding build configurations and integration test updates. The feedback focuses on improving the C++ implementation's robustness and resource management. Key recommendations include avoiding manual heap allocation of HttpsCallableReference by capturing it by value in the completion lambda, refactoring the timeout thread to use a std::condition_variable instead of std::this_thread::sleep_for to release resources immediately upon completion, and correcting a compiler definition syntax in CMakeLists.txt to prevent double-prefixing.
Dart sends a 70s default timeout, so every call parked its deadline thread (holding the reply callback) for the full duration. A condition variable notified from the completion handler lets the thread exit as soon as the request finishes. Addresses review feedback.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Windows support for the cloud_functions Flutter plugin, implementing the platform channel in C++ using the Firebase C++ SDK and updating integration tests accordingly. The review feedback highlights several critical issues in the C++ implementation, including a potential deadlock risk from invoking external callbacks while holding a mutex, potential undefined behavior when handling empty blobs or byte vectors with null pointers, and a potential linker error due to a non-inline function definition in a header file.
…x; guard zero-size blobs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Windows support for the cloud_functions plugin, implementing the native C++ plugin code and updating the integration tests to run on Windows while skipping unsupported desktop C++ SDK features (such as streaming callables and exception details propagation). Feedback on the implementation suggests refactoring the Call function in cloud_functions_plugin.cpp to comply with the repository style guide's 20-line limit, initializing HttpsCallableReference directly using the copy constructor, and capturing state by value instead of by reference in the predicate lambda to avoid lifetime issues.
| void CloudFunctionsPlugin::Call( | ||
| const flutter::EncodableMap& arguments, | ||
| std::function<void(ErrorOr<std::optional<flutter::EncodableValue>> reply)> | ||
| result) { |
There was a problem hiding this comment.
The Call function is currently around 130 lines long, which violates the repository style guide rule of keeping functions under 20 lines to maintain conciseness and readability. Consider refactoring this function by extracting logical parts (such as argument parsing, callable reference initialization, and timeout thread setup) into smaller helper functions.
References
- Functions should be kept under 20 lines to avoid verbosity and improve maintainability. (link)
| auto* ref = new HttpsCallableReference(); | ||
| if (!function_name.empty()) { | ||
| *ref = functions->GetHttpsCallable(function_name.c_str()); | ||
| } else if (!function_uri.empty()) { | ||
| *ref = functions->GetHttpsCallableFromURL(function_uri.c_str()); | ||
| } else { | ||
| delete ref; | ||
| result(MakeFunctionsError( | ||
| "invalid-argument", | ||
| "Either functionName or functionUri must be provided")); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Instead of default-constructing HttpsCallableReference and then assigning to it, you can initialize it directly using the copy constructor. This is cleaner and avoids unnecessary default construction.
HttpsCallableReference* ref = nullptr;
if (!function_name.empty()) {
ref = new HttpsCallableReference(functions->GetHttpsCallable(function_name.c_str()));
} else if (!function_uri.empty()) {
ref = new HttpsCallableReference(functions->GetHttpsCallableFromURL(function_uri.c_str()));
} else {
result(MakeFunctionsError(
"invalid-argument",
"Either functionName or functionUri must be provided"));
return;
}| if (!state->cv.wait_for(lock, std::chrono::milliseconds(timeout_ms), | ||
| [&state] { return state->responded; })) { |
There was a problem hiding this comment.
Capture state by value ([state]) instead of by reference ([&state]) in the predicate lambda. Since state is a std::shared_ptr, capturing it by value is cheap, safe, and avoids any potential compiler warnings or lifetime concerns with the outer lambda's stack frame.
| if (!state->cv.wait_for(lock, std::chrono::milliseconds(timeout_ms), | |
| [&state] { return state->responded; })) { | |
| if (!state->cv.wait_for(lock, std::chrono::milliseconds(timeout_ms), | |
| [state] { return state->responded; })) { |
|
Hi @akshaynexus, thanks for the contribution. It looks like a few CI checks are failing. Could you kindly take a look when you have a chance? |
2 participants
Description
Implements Cloud Functions for Windows desktop over the Firebase C++ SDK, filling in the
CloudFunctionsHostApipigeon interface whose Windows codegen (windows/messages.g.*) already existed in the repo but had no host implementation.This mirrors the Linux implementation from #18461 (tested working there against the emulator on real hardware); the two are structurally parallel so a later shared-core extraction can cover both.
What's included
httpsCallable/httpsCallableFromUrlwith fullfirebase::Variant<->EncodableValueconversion (including typed-data lists)platformExceptionToFirebaseFunctionsExceptionexpectsdeadline-exceededwhile the SDK completion only cleans uporiginargument.stream()) reportunimplemented— the C++ SDK has no streaming APIe2e
cloud_functionsis added to the Windows case of the tests-app e2e suite. Skips are limited to genuine C++ SDK gaps (streaming callables, exceptiondetailspayload), and the timeout test drains its uncancellable request afterwards so the next test isn't queued behind it.Related Issues
Companion to #18461 (Linux desktop support).
Checklist