containerd / ttrpc-rust

Rust implementation of ttrpc (GRPC for low-memory environments)
Apache License 2.0
195 stars 45 forks source link

bugfix: still check request payload #229

Open abel-von opened 4 months ago

abel-von commented 4 months ago

the ttrpc golang do not send flagNodata in the request even it is an empty request.

fix #221

abel-von commented 4 months ago

/cc @wllenyj

wllenyj commented 4 months ago

We don't have a test case to verify this ok yet. Previously I was doing compatibility test in my locally. So some caution is needed.

Are there any compatibility issues between the current implementation and the Golang version?

abel-von commented 4 months ago

The ttrpc-rust checks if there is no data in the request by FLAG_NO_DATA set. but golang version do not set this flag even it has no data

 let no_data = (req_msg.header.flags & FLAG_NO_DATA) == FLAG_NO_DATA;

so even it is an empty request with no data, because golang version do not set FLAG_NO_DATA, we will send an empty message to the stream handler, which is not expected.

        if !no_data {
            // Fake the first data message.
            let msg = GenMessage {
                header: MessageHeader::new_data(stream_id, req.payload.len() as u32),
wllenyj commented 4 months ago

@Tim-Zhang Is there conflict this commit with the changes of #208 ?

abel-von commented 4 months ago

It seems golang version of ttrpc fix the empty payload by checking if the client stream is set. Shall rust version follow that logic?

abel-von commented 4 months ago

https://github.com/containerd/ttrpc/blob/v1.2.4/services.go#L147 @wllenyj @Tim-Zhang

abel-von commented 4 months ago

It seems we do not distinguish StreamingClient or StreamingServer in ttrpc-rust.

Tim-Zhang commented 4 days ago

@abel-von It does not work because !req.payload.is_empty() also filter default payload out, you can try it with

$ cargo run --example async-stream-server
// run in other terminal and it will be blocked
$ cargo run --example async-stream-client

@wllenyj It seems ttrpc-rust's handle of MESSAGE_TYPE_REQUEST is diff with ttrpc, I think the mechanism should be reviewd,.