ttrpc-rust icon indicating copy to clipboard operation
ttrpc-rust copied to clipboard

add accept method

Open KarstenB opened this issue 9 months ago • 7 comments

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 avatar Apr 09 '25 08:04 KarstenB

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

Tim-Zhang avatar Apr 29 '25 09:04 Tim-Zhang

Hi @KarstenB, Yes I agree with you that Server.start() hides potential errors under its spawning task. In https://github.com/containerd/ttrpc-rust/pull/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.

Tim-Zhang avatar Apr 29 '25 09:04 Tim-Zhang

+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 avatar May 05 '25 23:05 yonch

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.

yonch avatar May 07 '25 16:05 yonch

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.

KarstenB avatar May 07 '25 21:05 KarstenB

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.

Tim-Zhang avatar Jul 11 '25 02:07 Tim-Zhang

Is there anything else that should be changed, or can this be merged now?

KarstenB avatar Jul 23 '25 16:07 KarstenB