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 timing issue of streaming #220

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Conversation

abel-von
Copy link
Contributor

The stream request and data is handle asynchronously, if the data is handled when stream is not created yet, the data will be discarded and an error occur.

we should wait until the stream is created and then the subsequent message can be handled.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

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

Project coverage is 25.22%. Comparing base (a9198f2) to head (431bfb8).
Report is 11 commits behind head on master.

❗ Current head 431bfb8 differs from pull request most recent head 59c3b3e. Consider uploading reports for the commit 59c3b3e to get more accurate results

Files Patch % Lines
src/asynchronous/server.rs 0.00% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
+ Coverage   24.97%   25.22%   +0.25%     
==========================================
  Files          16       16              
  Lines        2651     2704      +53     
==========================================
+ Hits          662      682      +20     
- Misses       1989     2022      +33     

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

@jsturtevant
Copy link
Collaborator

jsturtevant commented Mar 21, 2024

looks like ci is failing with #219

the stream request and data is handle asynchronously, if the data is
handled when stream is not created yet, the data will be discarded and
an error occur.

Signed-off-by: Abel <[email protected]>
@abel-von
Copy link
Contributor Author

code is rebased, please take a look. and CI error of Coverage seems a network error? @jsturtevant

@@ -381,12 +381,14 @@ impl ReaderDelegate for ServerReader {
async fn handle_msg(&self, msg: GenMessage) {
let handler_shutdown_waiter = self.handler_shutdown.subscribe();
let context = self.context();
let (wait_tx, wait_rx) = tokio::sync::oneshot::channel::<()>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More make sence to use tokio::sync::Barrier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Barrier is for blocking a number of tasks, but here we just need to block the current task, it seems the oneshot channel is a simpler choice, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use tokio::sync::Notify, but as it do not implement Drop so that we have to call notify_one() in a lot of places otherwise the handle_msg method of ServerReader will be blocked if we miss calling notify_one() in any code branches, hence I think it is easy to introduce bugs.
I think the oneshot::channel is a better choice because whenever it is droped, the wait of it will be unblocked. so we don't have to call drop everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oneshot is ok in my opinion

abel-von added a commit to kuasar-io/ttrpc-rust that referenced this pull request Jul 30, 2024
Copy link
Member

@Tim-Zhang Tim-Zhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abel-von Good catch, thanks

@Tim-Zhang Tim-Zhang requested a review from wllenyj September 25, 2024 11:29
@wllenyj wllenyj merged commit 20db6ab into containerd:master Sep 26, 2024
10 of 11 checks passed
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.

4 participants