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

Fix the stream sending reliability #222

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

Conversation

abel-von
Copy link
Contributor

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.

Copy link

codecov bot commented Mar 14, 2024

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.

❗ 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 ⚠️
src/asynchronous/connection.rs 0.00% 6 Missing ⚠️
src/asynchronous/client.rs 0.00% 3 Missing ⚠️
src/asynchronous/server.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jsturtevant
Copy link
Collaborator

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

// It verifies PR #208
async fn echo_default_value(

@jsturtevant
Copy link
Collaborator

looks like ci is failing with #219

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. This commit adds a channel to the
message so that we can wait until the message is truely written to the
connection.

Signed-off-by: Abel Feng <fshb1988@gmail.com>
@abel-von abel-von force-pushed the fix-reliability branch 2 times, most recently from c7fdd33 to 3355f7d Compare May 13, 2024 03:46
@abel-von
Copy link
Contributor Author

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

// It verifies PR #208
async fn echo_default_value(

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

looks like ci is failing with #219

I think it is now fixed.

@wllenyj
Copy link
Collaborator

wllenyj commented May 15, 2024

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

abel-von commented May 27, 2024

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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants