Skip to content
New issue

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

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

Already on GitHub? # to your account

bugfix: still check request payload #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abel-von
Copy link
Contributor

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

fix #221

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

Signed-off-by: Abel Feng <fshb1988@gmail.com>
@abel-von
Copy link
Contributor Author

/cc @wllenyj

@wllenyj
Copy link
Collaborator

wllenyj commented May 14, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

wllenyj commented May 15, 2024

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

@abel-von
Copy link
Contributor Author

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
Copy link
Contributor Author

@abel-von
Copy link
Contributor Author

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

@Tim-Zhang
Copy link
Member

Tim-Zhang commented Sep 25, 2024

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ttrpc golang stream request with no FLAG_NO_DATA set
3 participants