ttrpc-rust icon indicating copy to clipboard operation
ttrpc-rust copied to clipboard

bugfix: still check request payload

Open abel-von opened this issue 9 months ago • 8 comments

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

fix #221

abel-von avatar May 13 '24 04:05 abel-von

/cc @wllenyj

abel-von avatar May 13 '24 04:05 abel-von

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?

wllenyj avatar May 14 '24 01:05 wllenyj

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),

abel-von avatar May 14 '24 02:05 abel-von

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

wllenyj avatar May 15 '24 02:05 wllenyj

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 avatar May 27 '24 03:05 abel-von

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

abel-von avatar May 27 '24 03:05 abel-von

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

abel-von avatar May 27 '24 04:05 abel-von

@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,.

Tim-Zhang avatar Sep 25 '24 15:09 Tim-Zhang