Managed gRPC client

[JamesNK]

Addresses #43

To do:

• Validating content type
• Deadline (client timeout)
• Deadline (copying C-core HTTP request header formatting)
• CallOptions.CancellationToken

Future:

• Compression
• CallOptions.WriteOptions
• CallOptions.PropagationToken
• CallOptions.Credentials
• CallOptions.IsWaitForReady

[JunTaoLuo]

Initial comments. Still reviewing.

[JunTaoLuo]

We need a better way to share these constants between the projects.

[JunTaoLuo]

Same here.

[rynowak]

The world is a better place when you just drop the word "helpers" from these apis. 😆

[JunTaoLuo]

Looks good overall.

[jtattermusch]

I think this is mostly looking good, but I'd still like to give it a closer look next week. If this PR is blocking your other work though, feel free to merge after addressing comments.

[JamesNK]

Big update. @jtattermusch Can you review the interop test client? I'd like to merge this and get the HttpClient gRPC client running in your interop environment ASAP. If there are failures caused by other servers, and the problem is HttpClient then we need to get issues raised to the HttpClient team quickly.

Interop test status:

• "empty_unary"
• "large_unary"
• "client_streaming"
• "server_streaming"
• "ping_pong" - Fails
• "empty_stream"
• "compute_engine_creds" - Fails
• "jwt_token_creds" - Fails
• "oauth2_auth_token" - Fails
• "per_rpc_creds" - Fails
• "cancel_after_begin"
• "cancel_after_first_response" - Fails
• "timeout_on_sleeping_server"
• "custom_metadata"
• "status_code_and_message"
• "unimplemented_service"
• "unimplemented_method"
• "client_compressed_unary" - Fails
• "client_compressed_streaming" - Fails

[JamesNK]

Updated the SDK so HttpClient supports making HTTP/2 requests without TLS.

Lots of changes

[rynowak]

What's the reason to use this instead of .CancelAfter?

[JamesNK]

A different status is returned depending on whether the call was cancelled via a token or via the deadline being exceeded. A CTS doesn't provide a reason why it was cancelled (Cancel vs CancelAfter)

[halter73]

It might be better to construct a new CTS with the specified timeout and register DeadlineExceeded with that instead of using a timer. That way when you dispose the registration, you wait for DeadlineExceeded to complete (if it's currently executing) automatically.

[rynowak]

It's not super clear to me what the threading model is for this (since you've got cancellation and timeouts). How does this work without locking?

[halter73]

I'm concerned about this too.

As long as the GrpcCall object itself is accessed from a single logical thread and then disposed after it's used, _ctsRegistration?.Dispose() should block until any possibly-already-triggered calls to CancelCall complete. This effectively locks on our behalf.

I don't know if _deadlineTimer?.Dispose() will similarly wait for DeadlineExceeded to complete. I don't think it does. It would be tempting to use a lock in Dispose and DeadlineExceeded to protect against ODE's, but you risk deadlocking if you're not extremely careful.

After a lot of effort trying to properly dispose the CTS backing the RequestAborted token without ODEs or deadlocking, @jkotalik and I found the best way to avoid deadlocking is to simply not even try disposing CTS that's already been canceled. dotnet/aspnetcore#9333

[halter73]

I don't think you need to do anything as complicated as what we did for the RequestAborted token since you don't try to lazily allocate the CTS nor do you expose the CancellationToken to the end user.

I do think that using a CTS instead of a Timer for DeadlineExceeded might help you avoid an ODE in DeadlineExceeded though.

[JamesNK]

Dispose, cancellation and error handling is immature at the moment.

Dispose can happen 3 ways:

1. Response headers are invalid (bad HttpStatusCode or bad content type)
2. Response is received and we finish reading the body
3. User explicitly calls dispose on the public API, which gets routed here

Then there are additional ways _callCts can be canceled:

1. The deadline could be exceeded
2. The user's passed in cancellation token is cancelled

There definitely needs to be some changes here to avoid cancelling an already disposed _callCts.

[JunTaoLuo]

Implementation looks good, going to look through the tests before signing off.

[JunTaoLuo]

Is this threadsafe?

[JamesNK]

Merging so this can be setup in the interop tests.

Still work to do in follow up PRs to get thread-safety perfect but that can be handled in follow up work.

Thanks for everyone's feedback.