-
Notifications
You must be signed in to change notification settings - Fork 60
add accept method #292
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
base: master
Are you sure you want to change the base?
add accept method #292
Conversation
5ac896b
to
12da424
Compare
src/asynchronous/server.rs
Outdated
@@ -171,6 +171,15 @@ impl Server { | |||
Ok(()) | |||
} | |||
|
|||
pub fn accept(&mut self, conn: Socket) -> Connection<impl Builder> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think name it 'accept' is not that suitable because the input parameter conn is produced by the traditional accept(2).
It looks like this function will run after the accept(2). How about calling it like 'new_connection'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I had serve_connected
, but I liked accept
because it was the second thing I thought of. While it is not a regular socket accept, it represents the functionality of an accept, but on a higher level. new_connection
would also be fine for me.
@KarstenB Please fix the CI by commit with the sign-off by using 'git commit -s' |
Hi @KarstenB, Yes I agree with you that Server.start() hides potential errors under its spawning task. How about adding a |
117e5f8
to
33c0348
Compare
+1 on the functionality. I'm also developing an NRI plugin and this will enable use in that setting. Thank you all for working on this! |
Note that the recent commit 33c0348 with
However the first suggested commit 8ac79f1 with the |
Ups, you're right. I forgot to push my update when I attempted to fix the sign-off. The problem is that the return |
Hi @KarstenB, how about squashing the 4 commits to one commit? by the way please add the commit body for the CI https://github.com/containerd/ttrpc-rust/actions/runs/15211814262/job/42787548995?pr=292 |
Add accept method for better error handling and ergononics when only one connection is expected. Fixes containerd#293 Signed-off-by: Karsten Becker <567973+KarstenB@users.noreply.github.com> Co-authored-by: Jorge Prendes <jorge.prendes@gmail.com>
48b05c1
to
2ec19bb
Compare
For my NRI plugin I only have a single connection. Wrapping that connection into a stream is possible, but comes with some significant disadvantages. The first being that the spawning hides any potential errors, the other my multiplexer needs to have a 'static lifetime. With the given PR a new method is introduced that directly runs the connection without "accepting" a new connection from the stream, thus allowing direct control over the threading and error handling.
Fixes #293