feat: add accept method (supersedes #292) - #314
Conversation
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>
| pub async fn accept(&self, conn: Socket) -> std::io::Result<()> { | ||
| let delegate = ServerBuilder { | ||
| services: self.services.clone(), | ||
| streams: Arc::default(), |
There was a problem hiding this comment.
style nit: spawn_connection_handler initializes the streams as
streams: Arc::new(Mutex::new(HashMap::new())), so you could consider doing the same here.
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn accept(&self, conn: Socket) -> std::io::Result<()> { |
There was a problem hiding this comment.
The crate defines it's own error type in here:
https://github.com/containerd/ttrpc-rust/blob/master/src/error.rs#L66
Every other public method on Server returns this crate-level Result<()> so can we use this instead?
From a consumer's perspective, if you're using this server and calling multiple methods, you'd expect a consistent error type.
| pub async fn accept(&self, conn: Socket) -> std::io::Result<()> { | |
| pub async fn accept(&self, conn: Socket) -> Result<()> { |
Tim-Zhang
left a comment
Tim-Zhang
left a comment
There was a problem hiding this comment.
These two issues prevent the new API from providing the advertised error handling and task control. Non-blocking: since the socket is already connected, serve_connection would describe the operation more accurately than accept. Also, spawn_connection_handler has no await point of its own, so it can be a regular function and its call-site .await can be removed.
| streams: Arc::default(), | ||
| shutdown_waiter: self.shutdown.subscribe(), | ||
| }; | ||
| Connection::new(conn, delegate).run().await |
There was a problem hiding this comment.
Propagate connection failures from accept
Connection::run() handles GenMessageError::InternalError by disconnecting and breaking, then unconditionally returns Ok(()). Read and decoding failures therefore remain hidden, defeating this API’s stated error-handling purpose. Preserve the termination error through cleanup and return it here; writer failures should also be surfaced.
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn accept(&self, conn: Socket) -> std::io::Result<()> { |
There was a problem hiding this comment.
Return an owned Send + 'static future
This async fn holds &Server for the connection lifetime. Because Server contains a non-Sync Listener, the returned future is not Send and cannot be passed to tokio::spawn. Clone the required state before returning an async move future, with impl Future<Output = Result<()>> + Send + 'static; no boxing is needed.
3 participants
Add accept method for better error handling and ergononics when only one connection is expected. Fixes #293
I didn't want to force push on a branch that is already used externally, but this branch removes the
&mut selfin favor of a&self.