Server-side metadata for all call types

christian-oudard commented 3 years ago

Related features: See issue #460

Description of feature: Implement metadata sent from the server and read by the client.

Implementation: ClientUnaryReceiver gains a .headers() and .trailer() method to convey metadata sent by server.

Checklist:

[x] cargo fmt -- --check passes.
[x] cargo +nightly clippy has no warnings.
[x] cargo test passes.

BusyJay commented 3 years ago

Thanks! You need to get all your commits signed off to pass the DOC check.

christian-oudard commented 3 years ago

@BusyJay, I implemented your suggestion, with a couple of snags. First, the function to take the metadata out of the context requires a reference to the underlying C struct, not to the rust Metadata struct. Is there a way to do this without making it a public tuple member how I did?

Also, the client segfaults when freeing memory at the end of program execution. I think that context is freeing a null pointer. Can you help debug this? You can reproduce the problem with the test code here: https://github.com/christian-oudard/rust_tutorial/tree/master/grpc_tutorial

BusyJay commented 3 years ago

...without making it a public tuple member

It's defined as C representation, so it's safe to cast *mut Metadata to *mut grpc_metadata_array.

the client segfaults when freeing memory at the end of program execution

You can run it with gdb, it will pause when core. It should provide helpful stacktrace for what's going wrong.

christian-oudard commented 3 years ago

It's defined as C representation, so it's safe to cast *mut Metadata to *mut grpc_metadata_array.

There does not appear to be any way to move out of the C struct without segfaulting, because of the use of Unref in the grpc C code. Please correct me if I'm wrong about this. I just implemented it with cloning the metadata instead of attempting to move it.

BusyJay commented 3 years ago

because of the use of Unref in the grpc C code.

Where did it called? Last time I checked, all it needs was just a pointer to an initialized struct.

In my assumption, it should be written like following:

let mut metadata = MetadataBuilder::new().build();
unsafe {
    grpcwrap_batch_context_take_initial_metadata(ctx, &mut metadata as *mut _ as *mut grpc_metadata_array);
}

christian-oudard commented 3 years ago

Alright, that's server-side metadata implemented for all four call types. @BusyJay, Can you re-review?

christian-oudard commented 3 years ago

@BusyJay, One more question about the design of the new headers API: Our usage of gRPC in the MobileCoin codebase relies exclusively on unary calls, with no asynchrony. Can you suggest an API to return headers without requiring users to convert to async?

The previous fork of grpc-rs we were using had a syntax like this: let (headers, hello_response, trailer) = client.hello_full(hello_request)

This required codegen changes to add a _full method on generated clients. Should we re-implement this method, or do you have a suggestion for an improvement to this API?

BusyJay commented 3 years ago

Our usage of gRPC in the MobileCoin codebase relies exclusively on unary calls, with no asynchrony.

Is it acceptable to use block_on explicitly when calling async method? I'm afraid generating too many methods can be too verbose.

christian-oudard commented 3 years ago

I do not understand why the unittests are failing. Can you help?

BusyJay commented 3 years ago

I can see CI is reporting compilation errors. You may need to update the usage to use latest APIs.

jcape commented 3 years ago

@BusyJay Hi, thanks for all you work on this project. I can appreciate the desire for a smaller API, but it is pretty obtuse to have headers only partially supported in the synchronous API, and require otherwise synchronous apps to write glue to use asynchronous headers...

BusyJay commented 3 years ago

and require otherwise synchronous apps to write glue to use asynchronous headers...

Good point. There are three options:

Adjust the generated API to return a struct named Response, which contains headers, messages and trailers. So all users of synchronous API have to adjust their existing code to get message explicitly.
Keep current style, and anyone interested in headers or trailers should use asynchronous APIs. Asynchronous API is more powerful and performant for serious usages.
Keep the current style and generate a new API to return the struct described in option 1.

Synchronous APIs are supposed to be a convenient wrappers of the asynchronous version, any further wrappers will just add extra complexity to code. Hence I'm in favor of either of the first two options. But I also want to hear more voices from others.

Any thoughts? /cc @kennytm @sticnarf @hunterlxt

kennytm commented 3 years ago

i prefer options 2 or 3.

christian-oudard commented 3 years ago

Regarding option 2, here is the sort of code we are currently writing to use the async API in a synchronous manner:

            let mut receiver = this
                .blockchain_api_client
                .get_last_block_info_async_opt(&Empty::new(), call_option)?;
            let (header, message, trailer) = block_on(async {
                (
                    receiver.headers().await.ok().cloned(),
                    receiver.message().await.expect("no message"),
                    receiver.trailer().await.ok().cloned(),
                )
            });

Is this the sort of usage you would like to promote for synchronous clients who need to get server headers?

sticnarf commented 3 years ago

let (header, message, trailer) = block_on(async {
    (
        receiver.headers().await.ok().cloned(),
        receiver.message().await.expect("no message"),
        receiver.trailer().await.ok().cloned(),
    )
});

Maybe this can be provided by ClientUnaryReceiver for convenience.

christian-oudard commented 3 years ago

I implemented @sticnarf 's suggestion, creating a method on ClientUnaryReceiver called receive_sync. Thoughts?

BusyJay commented 2 years ago

@christian-oudard do you have time to address comments? If not, I can help you to fix comments and get it merged.

christian-oudard commented 2 years ago

@christian-oudard do you have time to address comments? If not, I can help you to fix comments and get it merged.

Right now I don't have very much time to work on this. Last I looked at this PR, I was blocked on trying to find a place to split the C changes from the API changes. The differences in grcp_wrap.cc cause significant changes to internal function signatures, and I wasn't able to find an obvious place to stop propagating these changes.

BusyJay commented 2 years ago

I continue your work in #555. I don't push directly to your branch as it may be used for your business.

BusyJay commented 2 years ago

Close as #555 is merged.

tikv / grpc-rs

Server-side metadata for all call types #543