Skip to content

sync: support cancel_rx in ttrpc context#197

Merged
Tim-Zhang merged 2 commits intocontainerd:masterfrom
yuqitao:support-cancel_rx
Jul 6, 2023
Merged

sync: support cancel_rx in ttrpc context#197
Tim-Zhang merged 2 commits intocontainerd:masterfrom
yuqitao:support-cancel_rx

Conversation

@yuqitao
Copy link
Copy Markdown
Contributor

@yuqitao yuqitao commented Jun 24, 2023

Fixes: #180

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.30 ⚠️

Comparison is base (e94bb9f) 24.47% compared to head (f9bff19) 24.17%.

❗ Current head f9bff19 differs from pull request most recent head 100cff9. Consider uploading reports for the commit 100cff9 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   24.47%   24.17%   -0.30%     
==========================================
  Files          17       17              
  Lines        2521     2552      +31     
==========================================
  Hits          617      617              
- Misses       1904     1935      +31     
Impacted Files Coverage Δ
src/sync/server.rs 0.00% <0.00%> (ø)
src/sync/utils.rs 0.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yuqitao yuqitao force-pushed the support-cancel_rx branch from 9def45a to f9bff19 Compare June 24, 2023 13:49
async-trait = { version = "0.1.31", optional = true }
tokio = { version = "1", features = ["rt", "sync", "io-util", "macros", "time"], optional = true }
futures = { version = "0.3", optional = true }
crossbeam = "0.8.0"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to use crossbeam here? The rest of the code uses the std::sync::mpsc::Receiver/Sender

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With crossbeam channel, we can use select! in ttrpc method handler.
like:

select! {
       recv(ticker) -> _ => println!("elapsed: {:?}", start.elapsed()),
       recv(timeout) -> _ => break,
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

interesting! seems like a possibly good improvement thought I would tend towards bring in crossbeam as dep in separate PR across all the areas where std::sync::mpsc::Receiver/Sender is used to keep it similiar throughout the codebase. I will defer to the maintainers though.

.send(())
.unwrap_or_else(|err| trace!("Failed to send {:?}", err));
trace!("Socket error send control_tx");
trace!("wrokload_rx recv error, send control_tx");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
trace!("wrokload_rx recv error, send control_tx");
trace!("workload_rx recv error, send control_tx");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@Tim-Zhang Tim-Zhang requested a review from lifupan July 3, 2023 03:23
@Tim-Zhang
Copy link
Copy Markdown
Member

Good catch @yuqitao, @lifupan PTAL.

@yuqitao yuqitao force-pushed the support-cancel_rx branch from f9bff19 to b743c4e Compare July 4, 2023 09:12
yuqitao added 2 commits July 4, 2023 17:20
just rustup format code

Signed-off-by: yuqitao <yuqitao1024@qq.com>
add cancel_rx in ttrpcContext which can help method.handler to
realize the client has been closed, like context.Done in golang ttrpc.

Signed-off-by: yuqitao <yuqitao1024@qq.com>
@yuqitao yuqitao force-pushed the support-cancel_rx branch from b743c4e to 100cff9 Compare July 4, 2023 09:21
@yuqitao
Copy link
Copy Markdown
Contributor Author

yuqitao commented Jul 4, 2023

@lifupan @jsturtevant PTAL.

@yuqitao
Copy link
Copy Markdown
Contributor Author

yuqitao commented Jul 6, 2023

@Tim-Zhang The CI seems to have some wired error, could you help solving it please?

Copy link
Copy Markdown
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.

LGTM, thanks @yuqitao

@Tim-Zhang Tim-Zhang merged commit e5e1dbe into containerd:master Jul 6, 2023
Tim-Zhang added a commit to Tim-Zhang/ttrpc-rust that referenced this pull request Aug 22, 2023
Cut the release for containerd#196, containerd#197, containerd#200, containerd#203, containerd#208

Signed-off-by: Tim Zhang <tim@hyper.sh>
@Tim-Zhang Tim-Zhang mentioned this pull request Aug 22, 2023
KarstenB pushed a commit to KarstenB/ttrpc-rust that referenced this pull request May 1, 2025
sync: support cancel_rx in ttrpc context
KarstenB pushed a commit to KarstenB/ttrpc-rust that referenced this pull request May 1, 2025
Cut the release for containerd#196, containerd#197, containerd#200, containerd#203, containerd#208

Signed-off-by: Tim Zhang <tim@hyper.sh>
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.

Need a mechanism which can help method.handler to realize the client has been closed

4 participants