Skip to content

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

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

Conversation

KarstenB
Copy link

@KarstenB KarstenB commented Apr 9, 2025

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

@KarstenB KarstenB changed the title add start_connected method add accept method Apr 10, 2025
@@ -171,6 +171,15 @@ impl Server {
Ok(())
}

pub fn accept(&mut self, conn: Socket) -> Connection<impl Builder> {
Copy link
Member

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'

Copy link
Author

@KarstenB KarstenB Apr 30, 2025

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.

@Tim-Zhang
Copy link
Member

@KarstenB Please fix the CI by commit with the sign-off by using 'git commit -s'

@Tim-Zhang
Copy link
Member

Hi @KarstenB, Yes I agree with you that Server.start() hides potential errors under its spawning task.
In #285 @jokemanfire and I discussed about the problem.

How about adding a serve method and it is a twin of the start method but only not use spawn.

@KarstenB KarstenB force-pushed the serve_connected branch 5 times, most recently from 117e5f8 to 33c0348 Compare May 1, 2025 15:31
@yonch
Copy link

yonch commented May 5, 2025

+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!

@yonch
Copy link

yonch commented May 7, 2025

Note that the recent commit 33c0348 with accept() that returns a Connection seems to trigger the compiler's trait checking (at least the way I've been using it, wasn't immediately apparent to me how to fix):

error[E0599]: the method `run` exists for struct `Connection<impl Builder>`, but its trait bounds were not satisfied
   --> crates/nri/tests/integration_test.rs:201:14
    |
201 |         conn.run().await
    |              ^^^ method cannot be called on `Connection<impl Builder>` due to unsatisfied trait bounds
    |
    = note: the following trait bounds were not satisfied:
            `<impl r#async::connection::Builder as r#async::connection::Builder>::Reader: r#async::connection::ReaderDelegate`
            `<impl r#async::connection::Builder as r#async::connection::Builder>::Reader: Send`
            `<impl r#async::connection::Builder as r#async::connection::Builder>::Reader: Sync`
            `<impl r#async::connection::Builder as r#async::connection::Builder>::Writer: r#async::connection::WriterDelegate`
            `<impl r#async::connection::Builder as r#async::connection::Builder>::Writer: Send`
            `<impl r#async::connection::Builder as r#async::connection::Builder>::Writer: Sync`

However the first suggested commit 8ac79f1 with the start_connected() method appears to work ok.

@KarstenB KarstenB force-pushed the serve_connected branch from de45a9f to 5a19abf Compare May 7, 2025 21:11
@KarstenB
Copy link
Author

KarstenB commented May 7, 2025

Note that the recent commit 33c0348 with accept() that returns a Connection seems to trigger the compiler's trait checking (at least the way I've been using it, wasn't immediately apparent to me how to fix):
However the first suggested commit 8ac79f1 with the start_connected() method appears to work ok.

Ups, you're right. I forgot to push my update when I attempted to fix the sign-off. The problem is that the return impl Builder is not sufficient to match the required traits of the Connection to call run(). I think returning std::io::Result<()> is the better solution here, which still gives control over cancelation via async.

@Tim-Zhang
Copy link
Member

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
Thanks.

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>
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.

Add single connection support to Server
4 participants