Skip to content

feat(functions): add Cloud Functions support for Windows desktop - #18471

Open
akshaynexus wants to merge 9 commits into
firebase:mainfrom
akshaynexus:add-windows-cloud-functions
Open

feat(functions): add Cloud Functions support for Windows desktop#18471
akshaynexus wants to merge 9 commits into
firebase:mainfrom
akshaynexus:add-windows-cloud-functions

Conversation

@akshaynexus

Copy link
Copy Markdown

Description

Implements Cloud Functions for Windows desktop over the Firebase C++ SDK, filling in the CloudFunctionsHostApi pigeon 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 / httpsCallableFromUrl with full firebase::Variant <-> EncodableValue conversion (including typed-data lists)
  • Canonical error-code mapping, with code/message in the error details map as Dart's platformExceptionToFirebaseFunctionsException expects
  • Per-call timeout enforced in the plugin via a deadline thread (the C++ SDK has no deadline API); responds deadline-exceeded while the SDK completion only cleans up
  • Emulator support via the origin argument
  • Streaming callables (.stream()) report unimplemented — the C++ SDK has no streaming API

e2e

cloud_functions is added to the Windows case of the tests-app e2e suite. Skips are limited to genuine C++ SDK gaps (streaming callables, exception details payload), 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

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes e2e test coverage for the new feature.
  • All existing and new tests are passing.

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.
@gemini-code-assist

Copy link
Copy Markdown
Contributor
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

@akshaynexus

Copy link
Copy Markdown
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 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.

Comment thread packages/cloud_functions/cloud_functions/windows/cloud_functions_plugin.cpp Outdated
Comment thread packages/cloud_functions/cloud_functions/windows/cloud_functions_plugin.cpp Outdated
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.
@akshaynexus

Copy link
Copy Markdown
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 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.

Comment thread packages/cloud_functions/cloud_functions/windows/cloud_functions_plugin.cpp Outdated
@akshaynexus

Copy link
Copy Markdown
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 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.

Comment thread packages/cloud_functions/cloud_functions/windows/CMakeLists.txt
Comment thread packages/cloud_functions/cloud_functions/windows/cloud_functions_plugin.cpp Outdated
Comment thread packages/cloud_functions/cloud_functions/windows/cloud_functions_plugin.cpp Outdated
Comment thread packages/cloud_functions/cloud_functions/windows/cloud_functions_plugin.cpp Outdated
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.
@akshaynexus

Copy link
Copy Markdown
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 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.

Comment thread packages/cloud_functions/cloud_functions/windows/cloud_functions_plugin.cpp Outdated
@akshaynexus

Copy link
Copy Markdown
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 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.

Comment on lines +236 to +239
void CloudFunctionsPlugin::Call(
const flutter::EncodableMap& arguments,
std::function<void(ErrorOr<std::optional<flutter::EncodableValue>> reply)>
result) {

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.

medium

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
  1. Functions should be kept under 20 lines to avoid verbosity and improve maintainability. (link)

Comment on lines +266 to +277
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;
}

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.

low

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;
  }

Comment on lines +315 to +316
if (!state->cv.wait_for(lock, std::chrono::milliseconds(timeout_ms),
[&state] { return state->responded; })) {

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.

low

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.

Suggested change
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; })) {

@SelaseKay

Copy link
Copy Markdown
Contributor

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?

result(MakeFunctionsError(code, message));
}
}
delete state->ref;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I tried merging this PR into a fork to use cloud functions in Windows, and this line crashed the app after any cloud function call. What ended up fixing the issue is wrapping the delete in a short-lived thread:

std::thread([ref = state->ref]() { delete ref; }).detach();

I honestly don't know much about native Windows code, but I just wanted to point this out as it crashes the app every single time in my tests.

@akshaynexus akshaynexus Sep 1, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok will fix that,this implementation was loosely ported from a linux implementation pr for firebase so some issues may have happened on windows that didn't happen on Linux

Thanks for the feedback

Upstream moved the Cloud Functions e2e suite into the package example app
and replaced the monolithic Windows workflow with per-product reusable
jobs, so the Windows wiring from this branch is ported to that layout:

- Drop .github/workflows/windows.yaml (deleted upstream) and add a
  `windows` job to e2e_tests_functions.yaml via reusable_e2e_windows.yaml.
- Scaffold a Windows runner for the cloud_functions example so the
  reusable job has something to `flutter drive`.
- Re-apply the desktop C++ SDK skips (no `details` on exceptions, no
  streaming callables, drain after timeout) on the moved e2e test and drop
  tests/integration_test/platform_utils.dart, whose only user moved.
- Register cloud_functions in the tests app's committed Windows plugin
  registrant, which the format check flagged on the PR.

Claude-Session: https://claude.ai/code/session_01M7EsbBhVWfzLhM3a9WTVsu
…mpletion thread

Deleting the HttpsCallableReference from inside its own OnCompletion
callback crashed the app after every call on Windows (reported on firebase#18471):
the completion runs inside the reference's Future/transport machinery, so
destroying it there tears down a live stack. Hand the delete to a
short-lived thread instead. The Windows CI job showed the same thing as a
non-zero exit at app teardown with every test green.

Also folds in the remaining review nits: construct the reference directly,
capture the shared state by value in the wait predicate, give the SDK a
non-null pointer for empty blobs, and hoist the per-call state, response
claiming, and timeout parsing out of Call() so the completion path no
longer duplicates the deliver-under-lock logic.

Claude-Session: https://claude.ai/code/session_01M7EsbBhVWfzLhM3a9WTVsu
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.

3 participants