Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify documentation for half-duplex behaviour #141

Open
AThilenius opened this issue Feb 20, 2024 · 7 comments
Open

Clarify documentation for half-duplex behaviour #141

AThilenius opened this issue Feb 20, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@AThilenius
Copy link

Describe the bug

Hi. I'm the author of axum-connect, which supports server-streaming RPC handlers. I am however running in circles trying to figure out what the official spec is for the request part of a server-streaming (IE. unary request, streaming response) RPC. I implemented axum-connect according to the official protocol spec, which defines an RPC call as

Request → Unary-Request / Unary-Get-Request / Streaming-Request
...
Response → Unary-Response / Streaming-Response

This implies, though does not actually state, that server-streaming RPC requests are unary (do not have envelops). However, unless I'm misusing it somehow, connect-es is enveloping the request JSON, despite defining the RPC type itself as a unary.

To Reproduce

Here is a minimal web client example of calling Eliza Introduce. The only interesting snippet is

import { createConnectTransport } from "@bufbuild/connect-web";
import { ElizaService } from "@buf/connectrpc_eliza.bufbuild_connect-es/connectrpc/eliza/v1/eliza_connect";
import { createPromiseClient } from "@bufbuild/connect";

const transport = createConnectTransport({
    baseUrl: "https://connect.build/demo",
});
const client = createPromiseClient(ElizaService, transport);

async function test() {
    for await (const response of client.introduce({ name: "Bob Ross", })) {
        console.log(response);
    }
}

void test();

It runs a CORS error if you point it to the demo URL, but you can see the request contains than envelope message. I would only expect this for a streaming request. Otherwise I would expect the request to be normal JSON.
image

@AThilenius AThilenius added the bug Something isn't working label Feb 20, 2024
@srikrsna-buf srikrsna-buf transferred this issue from connectrpc/connect-es Feb 20, 2024
@srikrsna-buf
Copy link
Member

Hey! Thank you for creating axum-connect, it is great to see a rust implementation!

The Streaming RPCs section outlines all the streaming RPC types. Server steaming, Client streaming, and Bidi Streaming all use enveloped messages.

Transferred the issue here to track the verbiage change in the outline section if needed.

@AThilenius
Copy link
Author

AThilenius commented Feb 20, 2024

Most likely I've made a bad assumption, there is unfortunately still a lot of ambiguity in the spec.

First Assumption

From the spec:

Streaming RPCs may be half- or full-duplex. In server streaming RPCs, the client sends a single message and the server responds with a stream of messages. In client streaming RPCs, the client sends a stream of messages and the server responds with a single message.

My interpretation of that is that a half-duplex streams will be comprised of a unary part (the "sends a single message") and a streaming part (the "server response with a stream of messages").

My extrapolation of that reading for server-streaming RPCs is that:

  • Calling-semantics would be unary (not enveloped) but as noted:
    • A connect+* content type must be present that "prevents HTTP clients unaware of Connect's semantics from interpreting a streaming error response, which uses an HTTP Status of 200 OK, as successful"
  • Response semantics are streaming (enveloped with flags and content length, 200-status, et. al).

Second Assumption

My second assumption is that I'm correctly understanding connect-es is indeed enveloping the unary part of half-duplex requests. Part of me wonders if I'm just "holding it wrong" still 🤔

Spec Ambiguity

The spec doesn't actually say either way, including in the ABNF, it depends on how you read it. If the interpretation is instead "all streaming RPCs, client-streaming, server-streaming and bidi-streaming use streaming wire semantics both directions" and for example server-streaming requests just happen to expect exactly one payloads in the stream before the HTTP request terminator, then that is not defined anywhere. Nor the semantics for degenerate cases like request payload timeouts (are they considered unary, or streaming), content-size disagreement, more than one payload, stream termination without HTTP request completion, et. al.

@emcfarlane
Copy link
Contributor

@AThilenius thanks for raising the issue, this also caught me by surprise when I first read through the spec and will look at making this more obvious. From the spec:

Streaming RPCs may be half- or full-duplex.

The terms "server streaming" and "client streaming" are half streaming protocols but the streaming RPC behaviour is defined for both the request and response. A half-streaming protocol is streaming. So there is no unary part. This looks like:

  • unary: single request and response without message enveloping
  • server streaming: a single enveloped message request with a stream of zero or more response messages
  • client streaming: a stream of requests met with single response in a streaming envelope
  • full duplex: stream of requests and responses intermix (requires http2)

@AThilenius
Copy link
Author

@emcfarlane Appreciate the response! It's really good to have a definitive answer, I'll get axum-connect patched soon. Apparently no one is using streaming in axum-connect yet (myself included), so it has gone unnoticed lol.

It's a really surprising answer for me though. I feel like it leaves so much undefined behavior for the single-message parts of the streams. Rust is suuuuper strict, so I'll have to pick what feels like sane defaults myself and define all the undefined behavior in the spec. C'est la vie.

@emcfarlane
Copy link
Contributor

You might find the new conformance tooling useful. It's just been developed to help implementers assert correctness: https://github.com/connectrpc/conformance

@AThilenius
Copy link
Author

You might find the new conformance tooling useful. It's just been developed to help implementers assert correctness: https://github.com/connectrpc/conformance

That is perfect, I've been wanting something like that since first creating axum-connect, didn't exist at the time though. Appreciate the link!

@AThilenius
Copy link
Author

As for this issue I'm good with either closing it (my question has been answered) or turning it into a 'improve the docs' tracking issue. Up to you guys.

@emcfarlane emcfarlane changed the title Server-streaming protocol clarification Clarify documentation for half-duplex behaviour Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants