GH-50282: [C++][FlightRPC] Refactor gRPC server and transport classes by Alex-PLACET · Pull Request #50407 · apache/arrow

Alex-PLACET · 2026-07-07T13:38:05Z

Rationale for this change

The goal of the pullrequest is to refactor the FligtRPC code to prepare the async implementation.

What changes are included in this PR

extracts server.cc/transport_server.cc internals into transport_server_internal.{h,cc} and the gRPC-specific call context, auth/middleware helper, and endpoint/builder setup into transport/grpc/grpc_server_internal.{h,cc}, templated on the gRPC context type so ServerContext and CallbackServerContext can share one implementation.
Moves signal-driven shutdown into internal::ServerSignalState, fixing teardown ordering so handlers are restored even when Serve() fails early, a SIGPIPE race when waking the signal thread, and shutdown of an uninitialized server clearing another serving instance's signal target. (Github Copilot issue report)
Adds public Location::uri() and Location::port(), and includes arrow/util/uri.h from types.h so the Result<util::Uri> return type is instantiable by consumers of the installed header. (Github Copilot issue report in the PR)
Wait() on an uninitialized server now returns Status::Invalid instead of dereferencing null. No other public behavior change. ServerTransport, TransportMessageReader/Writer, WriteDataStream and friends stay internal. (Github Copilot issue report in the PR)
Drops the unused gRPC FlightData message registry.

Are these changes tested?

Full CI tests
Adds tests covering signal handling and server middleware headers that depend on handler state.

Are there any user-facing changes?

New API: Location::uri() → Result<util::Uri>, Location::port() → int (-1 when absent)
Compilation fix: types.h now includes arrow/util/uri.h. Before, Result<util::Uri> was uninstantiable for anyone including only the installed header.
Behavior: Wait() on an uninitialized server returns Status::Invalid (was a null deref). Serve() restores prior signal handlers on failure. Shutdown() on an uninitialized server no longer disables signal shutdown for another serving instance, signal thread wake no longer risks SIGPIPE-killing the process. (Github Copilot issue report in the PR)

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

Human
AI
Reviewed before submission by:
Human
AI
[] Not reviewed

GitHub Issue: [C++][FlightRPC] Refactor gRPC server and transport classes #50282

github-actions · 2026-07-07T13:38:41Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2026-07-07T13:40:28Z

⚠️ GitHub issue #50282 has been automatically assigned in GitHub to PR creator.

Alex-PLACET · 2026-07-08T09:13:15Z

@raulcd Can you launch the CI please

Alex-PLACET · 2026-07-09T11:18:43Z

@raulcd Can you run the CI again please?

Alex-PLACET · 2026-07-20T08:25:53Z

@raulcd Can you launch the CI please

Alex-PLACET · 2026-07-21T11:02:50Z

@raulcd Can you launch the CI please (I hope it will be the last time)

raulcd · 2026-07-22T11:20:32Z

@raulcd Can you launch the CI please (I hope it will be the last time)

Sorry @Alex-PLACET , I missed this comment. Just kicked it

Alex-PLACET · 2026-09-02T09:19:00Z

Most of the code was moved from transport_server.cc

Alex-PLACET · 2026-09-02T09:24:33Z

+  std::vector<std::shared_ptr<ServerMiddleware>> middleware_;
+  std::unordered_map<std::string, std::shared_ptr<ServerMiddleware>> middleware_map_;
+  CallHeaders incoming_headers_;
+};


Moved from https://github.com/apache/arrow/pull/50407/changes/BASE..20c2325e0b8481adf97aeac53b815f2509b9f301#diff-b3f03a3354d6ebe8a7212c9fd37b8c4333357552947460a941b39777baa28b17L115

Alex-PLACET · 2026-09-02T09:44:46Z

-    // Allow uploading messages of any length
-    builder.SetMaxReceiveMessageSize(-1);
-
-    const std::string scheme = uri.scheme();
    int port = 0;
-    if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
-      std::stringstream address;
-      address << arrow::util::UriEncodeHost(uri.host()) << ':' << uri.port_text();
-
-      std::shared_ptr<::grpc::ServerCredentials> creds;
-      if (scheme == kSchemeGrpcTls) {
-        ::grpc::SslServerCredentialsOptions ssl_options;
-        for (const auto& pair : options.tls_certificates) {
-          ssl_options.pem_key_cert_pairs.push_back({pair.pem_key, pair.pem_cert});
-        }
-        if (options.verify_client) {
-          ssl_options.client_certificate_request =
-              GRPC_SSL_REQUEST_AND_REQUIRE_CLIENT_CERTIFICATE_AND_VERIFY;
-        }
-        if (!options.root_certificates.empty()) {
-          ssl_options.pem_root_certs = options.root_certificates;
-        }
-        creds = ::grpc::SslServerCredentials(ssl_options);
-      } else {
-        creds = ::grpc::InsecureServerCredentials();
-      }
-
-      builder.AddListeningPort(address.str(), creds, &port);
-    } else if (scheme == kSchemeGrpcUnix) {
-      std::stringstream address;
-      address << "unix:" << uri.path();
-      builder.AddListeningPort(address.str(), ::grpc::InsecureServerCredentials());
-      location_ = options.location;
-    } else {
-      return Status::NotImplemented("Scheme is not supported: " + scheme);
-    }


Moved to a dedicated function: AddServerListeningPort

Alex-PLACET · 2026-09-02T09:45:30Z

+Status AddServerListeningPort(const FlightServerOptions& options,
+                              const arrow::util::Uri& uri, ::grpc::ServerBuilder* builder,
+                              Location* location, int* port) {
+  const std::string scheme = uri.scheme();
+  if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
+    std::stringstream address;
+    address << arrow::util::UriEncodeHost(uri.host()) << ':' << uri.port_text();
+
+    std::shared_ptr<::grpc::ServerCredentials> creds;
+    if (scheme == kSchemeGrpcTls) {
+      ::grpc::SslServerCredentialsOptions ssl_options;
+      for (const auto& pair : options.tls_certificates) {
+        ssl_options.pem_key_cert_pairs.push_back({pair.pem_key, pair.pem_cert});
+      }
+      if (options.verify_client) {
+        ssl_options.client_certificate_request =
+            GRPC_SSL_REQUEST_AND_REQUIRE_CLIENT_CERTIFICATE_AND_VERIFY;
+      }
+      if (!options.root_certificates.empty()) {
+        ssl_options.pem_root_certs = options.root_certificates;
+      }
+      creds = ::grpc::SslServerCredentials(ssl_options);
+    } else {
+      creds = ::grpc::InsecureServerCredentials();
+    }
+    builder->AddListeningPort(address.str(), creds, port);
+    return Status::OK();
+  }
+  if (scheme == kSchemeGrpcUnix) {
+    std::stringstream address;
+    address << "unix:" << uri.path();
+    builder->AddListeningPort(address.str(), ::grpc::InsecureServerCredentials());
+    *location = options.location;
+    return Status::OK();
+  }
+  return Status::NotImplemented("Scheme is not supported: " + scheme);
+}


Moved from grpc_server.cc

Alex-PLACET · 2026-09-02T09:59:07Z

 }

-namespace {
-class TransportIpcMessageReader : public ipc::MessageReader {


Moved to transport_server_internal.cc

Alex-PLACET · 2026-09-02T11:30:39Z

Most of the code comes from server.cc

Alex-PLACET · 2026-09-02T11:36:16Z

Most of the code moved to grpc_server_internal cpp and h

Copilot

🔵 Needs a closer look

Two moderate issues remain in the header completeness and signal lifecycle test handling.

4 open issues

3 resolved since last review

_{Review effort: Lite}

raulcd

CI failures are unrelated. I have reviewed changes and validated the parts that have been moved around.
Thanks @Alex-PLACET for the PR LGTM.

@lidavidm can you take a look again?

@kou maybe you are interested in a quick look

kou

+1

Could you use "gRPC" not "GRPC" in PR title?

Is the current PR description updated? If not, could you update the PR description for the latest changes?

kou · 2026-10-21T06:25:40Z

+  /// \param[in] not_started_message Logged if shutdown is called before the server
+  /// starts. \param[in] shutdown_warning Logged if shutdown itself fails.
+  template <typename WaitFn, typename ShutdownFn>
+  Status Serve(WaitFn&& wait, ShutdownFn&& shutdown, const char* not_started_message,


It seems that not_started_message isn't used. Can we remove it?

👍 removed

lidavidm

LGTM. Thanks for working through this!

Copilot

🔵 Needs a closer look

Adapt FinishRequest status handling for grpc::CallbackServerContext before approval.

3 open issues

1 resolved since last review

Include complete Uri definition in types.h

_{Review effort: Lite}

Alex-PLACET · 2026-10-21T08:37:43Z

@kou The description is up to date now and I replaced the GRPC->gRPC in both the issue and this PR.

raulcd

None of the failures are related. The two comments have been tackled. I am going to merge and if there's any follow up I am happy to tackle it.

github-actions Bot added the awaiting review Awaiting review label Jul 7, 2026

github-actions Bot added Component: C++ Component: FlightRPC labels Jul 7, 2026

Alex-PLACET changed the title ~~GH-#50282: [C++][FlightRPC] Refactor GRPC server and transport classes~~ GH-50282: [C++][FlightRPC] Refactor GRPC server and transport classes Jul 7, 2026

Alex-PLACET force-pushed the refactor/internal-transport branch 2 times, most recently from 560bd8d to 20c2325 Compare July 23, 2026 11:41

raulcd reviewed Jul 31, 2026

View reviewed changes

Comment thread ci/docker/ubuntu-22.04-cpp.dockerfile Outdated

Comment thread cpp/src/arrow/flight/transport/grpc/customize_grpc.h Outdated

raulcd added the CI: Extra: Package: Linux Run extra Linux Packages CI label Jul 31, 2026

github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 31, 2026

Alex-PLACET commented Sep 2, 2026

View reviewed changes

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 2, 2026

Alex-PLACET commented Sep 2, 2026

View reviewed changes

Comment thread cpp/src/arrow/flight/transport_server_internal.h

Alex-PLACET Sep 2, 2026

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.

Most of the code comes from server.cc

Alex-PLACET commented Sep 2, 2026

View reviewed changes

Alex-PLACET marked this pull request as ready for review September 17, 2026 14:40

Copilot AI lite review requested due to automatic review settings September 17, 2026 14:40

Alex-PLACET requested review from assignUser and lidavidm as code owners September 17, 2026 14:40

Alex-PLACET and others added 4 commits October 20, 2026 12:51

fix

8d97ee3

fix

7cfc4a8

fix

8fdb757

doc

6c2c010

Alex-PLACET force-pushed the refactor/internal-transport branch from 6bafb10 to 6c2c010 Compare October 20, 2026 10:51

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 20, 2026

Copilot started reviewing on behalf of Alex-PLACET October 20, 2026 10:52

View session

Copilot AI reviewed Oct 20, 2026

View reviewed changes

raulcd approved these changes Oct 20, 2026

View reviewed changes

github-actions Bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 20, 2026

kou approved these changes Oct 21, 2026

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 21, 2026

lidavidm approved these changes Oct 21, 2026

View reviewed changes

github-actions Bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Oct 21, 2026

raulcd changed the title ~~GH-50282: [C++][FlightRPC] Refactor GRPC server and transport classes~~ GH-50282: [C++][FlightRPC] Refactor gRPC server and transport classes Oct 21, 2026

Alex-PLACET added 2 commits October 21, 2026 09:58

Remove unused parameter in ServerSignalState::Serve method

29c6d41

Remove unused Uri namespace and include statement in types.h

aa64e8a

Copilot AI lite review requested due to automatic review settings October 21, 2026 08:01

Copilot started reviewing on behalf of Alex-PLACET October 21, 2026 08:12

View session

Copilot AI reviewed Oct 21, 2026

View reviewed changes

raulcd approved these changes Oct 21, 2026

View reviewed changes

raulcd merged commit 373d64c into apache:main Oct 21, 2026
115 of 124 checks passed

raulcd removed the awaiting merge Awaiting merge label Oct 21, 2026

raulcd mentioned this pull request Oct 21, 2026

[C++][FlightRPC] Refactor gRPC server and transport classes #50282

Closed

github-actions Bot added the awaiting merge Awaiting merge label Oct 21, 2026

Conversation

Alex-PLACET commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR

Are these changes tested?

Are there any user-facing changes?

Was AI used for this PR?

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

Alex-PLACET commented Jul 8, 2026

Uh oh!

Alex-PLACET commented Jul 9, 2026

Uh oh!

Alex-PLACET commented Jul 20, 2026

Uh oh!

Alex-PLACET commented Jul 21, 2026

Uh oh!

raulcd commented Jul 22, 2026

Uh oh!

Uh oh!

Uh oh!

Alex-PLACET Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

raulcd left a comment

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Alex-PLACET commented Oct 21, 2026

Uh oh!

raulcd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Alex-PLACET commented Jul 7, 2026 •

edited

Loading

Alex-PLACET Sep 2, 2026 •

edited

Loading