Change plugin credentials API to support both sync and async modes #12374

markdroth · 2017-09-01T22:12:14Z

This fixes a pre-existing problem where plugin credentials would sometimes invoke the callback in the same thread, thus creating a new exec_ctx in an unsafe way. ~~It also helps pave the way for the call combiner change.~~

Julien: Please review all code, with particular emphasis on the API change.
Ken: Please review python changes.
Stanley: Please review PHP changes.
Alex: Please review csharp and ruby changes.
Michael: Please review node changes.

Please let me know if you have any questions. Thanks!

murgatroid99

Node changes look fine

grpc-kokoro · 2017-09-01T22:38:38Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-01T23:14:44Z

[microbenchmarks] No significant performance differences

kpayson64

Python content LGTM

stanley-cheung

PHP code LGTM. Tested some scenarios where these auth callbacks were invoked and the tests passed. Fixed some compile errors in markdroth#4. Please review. Thanks!

Fixed some PHP errors

markdroth · 2017-09-05T18:13:18Z

@murgatroid99: Just to confirm, am I correct that node will always run the callback in a different thread?

grpc-kokoro · 2017-09-05T18:40:44Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-05T19:13:39Z

[microbenchmarks] No significant performance differences

murgatroid99 · 2017-09-05T21:58:47Z

In Node, everything is run in the same thread, but the callback will be called outside of the call stack of the original call.

apolcyn · 2017-09-06T07:53:43Z

@markdroth and @jtattermusch PTAL for markdroth#5

ruby changes LGTM

Guarantee that c# auth callbacks are async to core

markdroth · 2017-09-06T21:57:57Z

@kpayson64, it looks like there are some Cython build failures due to not being able to find the GRPC_METADATA_CREDENTIALS_PLUGIN_SYNC_MAX definition:

https://grpc-testing.appspot.com/job/gRPC_pull_requests_linux/7639/testReport/junit/(root)/aggregate_tests/run_tests_python_linux_dbg_native/

Any idea what I need to do to resolve this? Thanks!

grpc-kokoro · 2017-09-06T23:45:14Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-07T00:12:06Z

[microbenchmarks] No significant performance differences

grpc-kokoro · 2017-09-07T21:11:06Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-08T16:05:51Z

[microbenchmarks] No significant performance differences

…s_api_fix

grpc-kokoro · 2017-09-11T19:13:01Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-11T19:36:57Z

[microbenchmarks] No significant performance differences

…s_api_fix

grpc-kokoro · 2017-09-25T19:59:33Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-25T20:55:03Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__2M_2_0.counters.old: 1


[microbenchmarks] Performance differences noted:
Benchmark                                                                                cpu_time    real_time
---------------------------------------------------------------------------------------  ----------  -----------
BM_PumpStreamServerToClient<SockPair>/512                                                            +4%
BM_StreamingPingPong<MinInProcess, NoOpMutator, NoOpMutator>/4k/2                        +6%         +6%
BM_StreamingPingPongWithCoalescingApi<InProcessCHTTP2, NoOpMutator, NoOpMutator>/64/1/1  +4%         +5%
BM_UnaryPingPong<MinInProcessCHTTP2, NoOpMutator, NoOpMutator>/0/8                       +4%         +4%

fengli79 · 2017-09-26T19:54:44Z

src/core/lib/security/credentials/plugin/plugin_credentials.c

@@ -125,6 +140,7 @@ static bool plugin_get_request_metadata(grpc_exec_ctx *exec_ctx,
                                        grpc_closure *on_request_metadata,
                                        grpc_error **error) {
  grpc_plugin_credentials *c = (grpc_plugin_credentials *)creds;


@markdroth Could you add traces to track the begin/end of the sync/async request? It helps to debug potential deadlock issues mentioned here: #11553 (comment)

grpc-kokoro · 2017-09-26T22:39:35Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-26T23:29:12Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__2M_2_0.counters.old: 1


[microbenchmarks] Performance differences noted:
Benchmark                                                                                            atm_add_per_iteration    cpu_time    nows_per_iteration    real_time
---------------------------------------------------------------------------------------------------  -----------------------  ----------  --------------------  -----------
BM_PumpStreamServerToClient<InProcess>/0                                                                                      +5%                               +6%
BM_PumpStreamServerToClient<SockPair>/512                                                                                                                       +9%
BM_StreamingPingPongMsgs<MinTCP, NoOpMutator, NoOpMutator>/2M                                        -5%                                  -6%
BM_StreamingPingPongWithCoalescingApi<MinInProcessCHTTP2, NoOpMutator, NoOpMutator>/128M/2/0                                                                    +5%
BM_UnaryPingPong<InProcess, NoOpMutator, Server_AddInitialMetadata<RandomAsciiMetadata<31>, 1>>/0/0                           +4%                               +5%
BM_UnaryPingPong<MinInProcess, NoOpMutator, NoOpMutator>/1/0                                                                  +4%                               +5%

jboeuf

LGTM. A few cosmetic comments. Thanks for the fix!

jboeuf · 2017-09-27T05:53:16Z

include/grpc/grpc_security.h

@@ -249,19 +249,40 @@ typedef struct {
  void *reserved;
 } grpc_auth_metadata_context;

+/** Maximum number of credentials returnable by a credentials plugin via


Maximum number of metadata as opposed to credentials.

jboeuf · 2017-09-27T23:20:04Z

src/cpp/client/secure_credentials.cc

    w->thread_pool_->Add(
        std::bind(&MetadataCredentialsPluginWrapper::InvokePlugin, w, context,
-                  cb, user_data));
+                  cb, user_data, nullptr, nullptr, nullptr, nullptr));
+    return false;


Since we're returning an int here, should this be 0 instead?

jboeuf · 2017-09-27T23:20:19Z

src/cpp/client/secure_credentials.cc

+    // Synchronous return.
+    w->InvokePlugin(context, cb, user_data, creds_md, num_creds_md, status,
+                    error_details);
+    return true;


fengli79 · 2017-09-27T23:30:31Z

include/grpc/grpc_security.h

@@ -249,19 +249,40 @@ typedef struct {
  void *reserved;
 } grpc_auth_metadata_context;

+/** Maximum number of credentials returnable by a credentials plugin via
+    a synchronous return. */
+#define GRPC_METADATA_CREDENTIALS_PLUGIN_SYNC_MAX 4


Can we make it configurable? Php need to get the callback always invoked on the same thread. Thus Php may use a larger threshold to make sure it's always sync.

No, we can't make it configurable. For performance reasons, we need to allocate space for the return parameters on the stack instead of heap-allocating it, so the number has to be set at compile time.

This limitation may go away in the future with some metadata API changes that @ctiller has in mind. But for now, if PHP needs to have a plugin return more then 4 metadata elements, then it needs to use the async API.

grpc-kokoro · 2017-09-28T16:09:10Z

[trickle] No significant performance differences

grpc-kokoro · 2017-09-28T16:51:04Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_streaming_ping_pong.BM_StreamingPingPongWithCoalescingApi_MinInProcessCHTTP2_NoOpMutator_NoOpMutator__2M_2_0.counters.old: 3


[microbenchmarks] Performance differences noted:
Benchmark                                                                                    nows_per_iteration
-------------------------------------------------------------------------------------------  --------------------
BM_StreamingPingPongWithCoalescingApi<MinInProcessCHTTP2, NoOpMutator, NoOpMutator>/16M/1/1  -4%

…s_api_fix

grpc-testing · 2017-09-28T22:39:51Z

[trickle] No significant performance differences

grpc-testing · 2017-09-28T23:19:40Z

[microbenchmarks] No significant performance differences

markdroth · 2017-09-29T14:26:30Z

Known issues: #11737 #12510 #11745 #12595 #12245

markdroth and others added 2 commits September 1, 2017 15:04

Change plugin credentials API to support both sync and async modes.

2caf021

Changes

d27504f

markdroth requested review from murgatroid99, jboeuf, apolcyn, stanley-cheung and kpayson64 September 1, 2017 22:12

googlebot added the cla: yes label Sep 1, 2017

murgatroid99 approved these changes Sep 1, 2017

View reviewed changes

kpayson64 approved these changes Sep 2, 2017

View reviewed changes

Fixed some PHP errors

6d8cc7c

stanley-cheung mentioned this pull request Sep 5, 2017

Fixed some PHP errors markdroth/grpc#4

Merged

stanley-cheung approved these changes Sep 5, 2017

View reviewed changes

Merge pull request #4 from stanley-cheung/plugin_credentials_api_fix

8cc7e67

Fixed some PHP errors

apolcyn and others added 3 commits September 6, 2017 14:12

make a guarantee that c# auth callbacks are async to c-core

27670c8

Merge pull request #5 from apolcyn/guarantee_csharp_creds_async

2c4e7d7

Guarantee that c# auth callbacks are async to core

clang-format

1ff6ee1

markdroth mentioned this pull request Sep 7, 2017

Second attempt at call combiner PR #12369

Merged

Build fixes

4fe48e6

kpayson64 approved these changes Sep 8, 2017

View reviewed changes

markdroth added 2 commits September 11, 2017 11:27

Merge remote-tracking branch 'upstream/master' into plugin_credential…

09fe5de

…s_api_fix

clang-format

ad9208c

RyanGordon mentioned this pull request Sep 18, 2017

Give ownership of byte buffers to write ops #12306

Merged

markdroth mentioned this pull request Sep 25, 2017

HHVM Extension Support #11553

Closed

Merge remote-tracking branch 'upstream/master' into plugin_credential…

b4c01f9

…s_api_fix

fengli79 reviewed Sep 26, 2017

View reviewed changes

Add tracer for plugin credentials.

1a8bb82

markdroth mentioned this pull request Sep 27, 2017

Data race in composite call credentials implementation #12581

Closed

jboeuf approved these changes Sep 27, 2017

View reviewed changes

fengli79 reviewed Sep 27, 2017

View reviewed changes

Code review changes.

61b26f9

Merge remote-tracking branch 'upstream/master' into plugin_credential…

6456e49

…s_api_fix

markdroth merged commit ede8ed2 into grpc:master Sep 29, 2017

markdroth deleted the plugin_credentials_api_fix branch September 29, 2017 14:26

nathanielmanistaatgoogle added the lang/Python label Sep 29, 2017

murgatroid99 mentioned this pull request Oct 2, 2017

Update grpc submodule grpc/grpc-node#55

Merged

lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change plugin credentials API to support both sync and async modes #12374

Change plugin credentials API to support both sync and async modes #12374

Change plugin credentials API to support both sync and async modes #12374

Change plugin credentials API to support both sync and async modes #12374

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment