containerd / ttrpc-rust

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

Fix the stream sending reliability #222

Closed abel-von closed 5 days ago

abel-von commented 6 months ago

Currently the send() method of stream implemented by send the value to an unbounded channel, so even the connection is closed for a long time, the send function still return succeed.

In this PR I add a channel to the message so that we can wait until the message is truely written to the connection.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 0% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 25.08%. Comparing base (a9198f2) to head (c09e90f). Report is 11 commits behind head on master.

:exclamation: Current head c09e90f differs from pull request most recent head 3355f7d. Consider uploading reports for the commit 3355f7d to get more accurate results

Files Patch % Lines
src/asynchronous/stream.rs 0.00% 27 Missing :warning:
src/asynchronous/connection.rs 0.00% 6 Missing :warning:
src/asynchronous/client.rs 0.00% 3 Missing :warning:
src/asynchronous/server.rs 0.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #222 +/- ## ========================================== + Coverage 24.97% 25.08% +0.11% ========================================== Files 16 16 Lines 2651 2719 +68 ========================================== + Hits 662 682 +20 - Misses 1989 2037 +48 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jsturtevant commented 6 months ago

Is this something that where check could be added in https://github.com/containerd/ttrpc-rust/blob/master/example/async-client.rs? Similar to what was done in https://github.com/containerd/ttrpc-rust/blob/6f12d9c49700e08a8669cc29108ff062b5dccf81/example/async-stream-server.rs#L138-L139

jsturtevant commented 6 months ago

looks like ci is failing with https://github.com/containerd/ttrpc-rust/issues/219

abel-von commented 4 months ago

Is this something that where check could be added in https://github.com/containerd/ttrpc-rust/blob/master/example/async-client.rs? Similar to what was done in

https://github.com/containerd/ttrpc-rust/blob/6f12d9c49700e08a8669cc29108ff062b5dccf81/example/async-stream-server.rs#L138-L139

I think it is hard to write test here because it is some exceptional cases, for example server side restart. or we can do it by disconnecting the underlying connection, but it seems we have no way to do it in client directly.

abel-von commented 4 months ago

looks like ci is failing with https://github.com/containerd/ttrpc-rust/issues/219

I think it is now fixed.

wllenyj commented 4 months ago

This would make send synchronous, and it feels like this behavior should be implemented by the upper level user. Or make it optional?

abel-von commented 4 months ago

I think maybe it is the best if ttrpc-rust behaves the same with https://github.com/containerd/ttrpc. The send in ttrpc golang seems synchronized as the channel is a blocking channel, as defined in https://github.com/containerd/ttrpc/blob/main/server.go#L338. so if underlying connection is closed, the send will also failed.

I think maybe we can not say this is "synchronized" as we don't really waiting for a acknowledgement from the other endpoint. we just make sure that the data is written into the connection.

Burning1020 commented 2 months ago

@wllenyj @jsturtevant Could you please take a look at these PRs?