Add binder server by TaWeiTu · Pull Request #27036 · grpc/grpc

TaWeiTu · 2021-08-17T09:17:18Z

In order for the server to listen for binder connections, the users should tell the server to listen on a "binder port" using the binder server credentials. A "binder port" is represented as "binder://" followed by the service name, for example,

grpc::ServerBuilder server_builder;
server_builder.AddListeningPort("binder://echo.service", grpc::experimental::BinderServerCredentials());

the server will now listen for binder connections to service "echo.service". The service name can be an arbitrary string and is used to retrieve the endpoint binder as follows

std::unique_ptr<grpc::Server> server = server_builder.BuildAndStart();
void* endpoint_binder = grpc::experimental::binder::GetEndpointBinder("echo.service");

The Java service can then pass this endpoint binder to clients.

Depends on #26970.

@ctiller
@sifmelcara

sifmelcara

Most of the code came from other pending PRs?

Will do a review again after other PRs get merged

src/core/ext/transport/binder/server/binder_server.h

TaWeiTu · 2021-08-19T02:57:15Z

Most of the code came from other pending PRs?

Yeah I have to build this PR on top of the other one in order to make the tests work. The relevant files in this PR are:

binder_server.h, binder_server.cc
binder_server_credentials.h, binder_server_credentials.cc
binder_server_test.cc

Will do a review again after other PRs get merged

Sure, no problem!

src/core/ext/transport/binder/server/binder_server_credentials.cc

TaWeiTu · 2021-08-25T12:25:15Z

Hi Craig, can you take a look at the overall API design of the binder server? It should be easier to review now since the gigantic PR it depends on has been merged. Thanks!

sifmelcara · 2021-08-26T21:13:13Z

src/core/ext/transport/binder/server/binder_server.h

+// TODO(waynetu): This is part of the public API and should be moved to the
+// include/ folder.


Can we just forward this header file in a header in include/ folder?

This should be in the include/ folder, but since we haven't had a discussion with gRPC folks about these "binder pool" APIs, I think it'd be better if we first agree on the overall design and then move these headers to a proper place.

sifmelcara · 2021-08-26T21:13:16Z

src/core/ext/transport/binder/server/binder_server.h

+// TODO(waynetu): Can these two functions be called in grpc_init() and
+// grpc_shutdown()?
+void grpc_endpoint_binder_pool_init();
+void grpc_endpoint_binder_pool_shutdown();


Yeah it is very inconvenient that user need to manually call this

I think we should probably make it being called exactly once in BinderServerListener's ctor (by using static variable initialization)

grpc_init is probably too general for this. What if user don't meant to use any binder functionality? Their CPU will wast time initiating our stuff

WDYT

Hmm. initializing the mutex and the object it protects at the same time is a bit tricky I think because mutex is not trivially destructible. IMHO putting them in grpc_init() and grpc_shutdown() (or functions they call) should be fine since it's just a few heap allocations at startup time. Probably need gRPC folks' opinion on whether this is acceptable and if so, where these functions should be called.

@ctiller @markdroth We'd like to have your opinions on whether these should be called in grpc_init() / grpc_shutdown() as well. Thanks!

@veblush what's holding up using absl::Mutex everywhere (which can be initialized statically?)

Pinging @veblush again

src/core/ext/transport/binder/server/binder_server_credentials.h

TaWeiTu · 2021-08-27T05:35:29Z

Moved BinderServerCredentials() to grpcpp/security/server_credentials.h (in experimental namespace). Also provide an experimental core API grpc_server_add_binder_port in grpc.h.

@ctiller and @markdroth can you take a look at the design here? Currently we're using AddListeningPort() with a custom URI scheme (binder://) as discussed in chat. We'd also like to have functions like GetEndpointBinder() for the users to retrieve the server endpoint binder after the server starts. Do you think it's OK to have such functions (and probably their C counterparts) in the public headers? If so, which file should they be moved to?

Thanks!

sifmelcara · 2021-08-27T21:31:41Z

TaWei: Any idea why Android (Internal CI) build suddenly cannot find NDK headers used in our headers?

TaWeiTu · 2021-08-28T02:40:38Z

TaWei: Any idea why Android (Internal CI) build suddenly cannot find NDK headers used in our headers?

I guess we were not building any Android code before in the CI since it's not included in tests, nor is it a target in BUILD and CMakeLists.txt. The Android docker only has ndk-bundle installed but not ndk;21.4.7075529, and I suppose that's the reason why the build failed. Not sure which of the following options is better:

Install ndk;21.4.7075529 in that docker
Instead of determining GPR_ANDROID or defined(ANDROID) || defined(__ANDROID__), additionally checks whether NDK binder is available.

sifmelcara · 2021-08-30T04:40:58Z

Ok then I think we should also check for binder API availability in the guard. Our code simply won't work with old version of NDK and API level.

I will do that

ctiller · 2021-08-30T16:44:47Z

include/grpc/grpc.h

+ *  binder transactions.
+ *  Returns 1 on success, 0 on failure (or called in non-Android environments)
+ *  REQUIRES: server not started */
+GRPCAPI int grpc_server_add_binder_port(grpc_server* server, const char* addr);


Public API additions will need a gRFC written.

I wonder if we still need this C API for us to wrap C++ API around it. According to https://github.com/grpc/grpc/projects/8 it seems no longer relevant.

Whilst I think it's probably a worthwhile idea, I don't think it's an actual official stance for the project (and if it were I think a gRFC would be needed for that).

We need a C API, and we need a gRFC for it.

Sure, we will include this C API into our gRFC.

ctiller · 2021-08-30T16:45:03Z

include/grpcpp/security/server_credentials.h

+/// Calling \a ServerBuilder::AddListeningPort() with Binder ServerCredentials
+/// in a non-Android environment will make the subsequent call to
+/// \a ServerBuilder::BuildAndStart() returns a null pointer.
+std::shared_ptr<ServerCredentials> BinderServerCredentials();


Public API's will need a gRFC.

We decided to temporarily move the APIs back to internal headers because we need this PR in the internal testings and the RFC process is likely to take longer than desired. We're drafting a gRFC, and will make the APIs public once the RFC is approved. Thanks!

ctiller · 2021-08-30T16:50:04Z

src/core/ext/transport/binder/server/binder_server.h

+};
+
+template <typename BinderTxReceiver>
+int AddBinderPort(const std::string& addr, grpc_server* server) {


Why not bool?

ctiller · 2021-08-30T16:51:15Z

src/core/ext/transport/binder/server/binder_server.h

+  std::unique_ptr<grpc_binder::TransactionReceiver> tx_receiver_;
+};
+
+template <typename BinderTxReceiver>


Make this template argument an interface instead.

I've made it into a lambda that constructs the receiver.

sifmelcara · 2021-09-01T19:16:56Z

Public APIs has been removed. We will post a gRFC today or tomorrow. Craig: We would like to merge this first to unblock our implementation of CI testing (both public and internal), and adjust/extend our interface during the RFC review. WDYT?

Also, make endpoint binder pool a gRPC core API so that it makes more sense to init/shutdown it in grpc_init() and grpc_shutdown(). However, grpc_endpoint_binder_pool_init() and grpc_endpoint_binder_pool_shutdown() are still called manually since it seems like grpc_init() only calls functions defined in src/core/lib.

* Make public APIs visible but function-less in non-Androind environments.

Add `grpc::BinderServerCredentials()` and other related functionalities for the server to listen to binder transactions through a phony "binder port". The APIs are temporarily placed in internal headers until the corresponding gRFC is merged.

TaWeiTu added the release notes: no Indicates if PR should not be in release notes label Aug 17, 2021

TaWeiTu changed the title ~~Add Binder server~~ Add binder server Aug 17, 2021

TaWeiTu force-pushed the binder-server branch from c2d80b3 to 9866a08 Compare August 18, 2021 04:17

TaWeiTu marked this pull request as ready for review August 18, 2021 08:37

TaWeiTu requested review from ctiller and sifmelcara August 18, 2021 08:37

sifmelcara reviewed Aug 19, 2021

View reviewed changes

src/core/ext/transport/binder/server/binder_server.h Outdated Show resolved Hide resolved

TaWeiTu force-pushed the binder-server branch 2 times, most recently from 7e81906 to fe7003f Compare August 19, 2021 03:30

sifmelcara reviewed Aug 20, 2021

View reviewed changes

src/core/ext/transport/binder/server/binder_server_credentials.cc Outdated Show resolved Hide resolved

TaWeiTu force-pushed the binder-server branch 5 times, most recently from 87f93f5 to 2346fc1 Compare August 24, 2021 07:51

markdroth assigned ctiller Aug 24, 2021

TaWeiTu force-pushed the binder-server branch from 2346fc1 to 6c46f5d Compare August 25, 2021 10:00

TaWeiTu force-pushed the binder-server branch from 6c46f5d to 0ff8805 Compare August 26, 2021 07:35

sifmelcara reviewed Aug 26, 2021

View reviewed changes

TaWeiTu force-pushed the binder-server branch from c96002b to 47909cd Compare August 27, 2021 05:27

ctiller reviewed Aug 30, 2021

View reviewed changes

TaWeiTu force-pushed the binder-server branch 3 times, most recently from 8ffd3d6 to 8569feb Compare September 1, 2021 06:23

TaWeiTu mentioned this pull request Sep 3, 2021

L85: Native cross-process communication on Android using BinderChannel grpc/proposal#259

Closed

ctiller approved these changes Sep 3, 2021

View reviewed changes

TaWeiTu force-pushed the binder-server branch from 8569feb to 5cc724d Compare September 3, 2021 17:12

TaWeiTu added 15 commits September 6, 2021 10:50

Add binder server

a97a67d

Commit tools/run_tests/generated/tests.json

e53a05e

Fix tests & sanity checks

e876fc5

Fix tests under TSAN

df74c70

Fix sanity checks

2340e29

Fix server->default_user_resource()

fef764f

Add missing include headers

94ab4a0

Rebase

0d69ebb

Add layering check

f01d188

Move BinderServerCredentials() to public headers

70eff8c

Fix build failures

22c7597

* Make public APIs visible but function-less in non-Androind environments.

Fix unused parameters

7a5d396

Remove the template parameter

4060da8

Move APIs back to internal headers

6c6b3dc

TaWeiTu force-pushed the binder-server branch from 5cc724d to 6c6b3dc Compare September 6, 2021 02:51

TaWeiTu enabled auto-merge (squash) September 6, 2021 02:51

sifmelcara disabled auto-merge September 6, 2021 03:00

Add missing dependency

77df10b

TaWeiTu enabled auto-merge (squash) September 6, 2021 05:01

TaWeiTu merged commit 3df113f into grpc:master Sep 6, 2021

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 7, 2021

		// TODO(waynetu): This is part of the public API and should be moved to the
		// include/ folder.

Conversation

TaWeiTu commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sifmelcara left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TaWeiTu commented Aug 19, 2021

Uh oh!

Uh oh!

TaWeiTu commented Aug 25, 2021

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!

Uh oh!

Uh oh!

TaWeiTu commented Aug 27, 2021

Uh oh!

sifmelcara commented Aug 27, 2021

Uh oh!

TaWeiTu commented Aug 28, 2021

Uh oh!

sifmelcara commented Aug 30, 2021

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!

TaWeiTu Sep 2, 2021 • 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!

TaWeiTu Aug 31, 2021 • 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!

sifmelcara commented Sep 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TaWeiTu commented Aug 17, 2021 •

edited

Loading

TaWeiTu Sep 2, 2021 •

edited

Loading

TaWeiTu Aug 31, 2021 •

edited

Loading