add accept method

[KarstenB]

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

[Tim-Zhang]

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'

[KarstenB]

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]

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

[Tim-Zhang]

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.

[yonch]

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

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]

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]

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.

[KarstenB]

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

[jprendes]

question: Why does this need to be &mut self, and not just &self?

Also, should this be an async function? or a normal function that returns a Connection (or some kind or wrapper)
As an async function, &mut self is captured until the connection ends.
IIUC, returning the connection would allow accepting more than one connection.

[KarstenB]

You mean something like this:

    pub fn accept(&self, conn: Socket) -> AcceptedConnection {
        let delegate = ServerBuilder {
            services: self.services.clone(),
            streams: Arc::default(),
            shutdown_waiter: self.shutdown.subscribe(),
        };
        AcceptedConnection(Connection::new(conn, delegate))
    }
    //...
    
pub struct AcceptedConnection(Connection<ServerBuilder>);

impl AcceptedConnection {
    pub async fn run(self) -> std::io::Result<()>{
        self.0.run().await
    }
}

This would also work for me. The point of the accept method in my use case is through I only have single connection anyhow. So I wasn't bothered by the &mut. Anyhow, a newtype wrapper is necessary otherwise a whole bunch of things would need to become public to be usable, which was the issue with your first proposal.

I have this implementation on my accepted branch https://github.com/KarstenB/ttrpc-rust/blob/accepted/src/asynchronous/server.rs#L174

[Tim-Zhang]

@KarstenB So why don't we change the &mut self to &self?

[KarstenB]

Addressed in #314. Please review there

[KarstenB]

Continued in #314