-
Notifications
You must be signed in to change notification settings - Fork 185
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
feat: refactor MakeTransport
and Incoming
#421
base: main
Are you sure you want to change the base?
Conversation
pub trait ShmExt: Send + Sync { | ||
fn release_read_and_reuse(&self) {} | ||
|
||
async fn close(&mut self) -> Result<(), anyhow::Error> { |
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.
Why use async-trait
and anyhow
, RPITIT and Box<dyn std::error::Error>
will also work.
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.
async-trait
is for the Box<dyn ShmExt>
usage, anyhow
I think it is good enough?
@@ -46,10 +49,14 @@ impl From<TcpListener> for DefaultIncoming { | |||
} | |||
|
|||
pub trait Incoming: fmt::Debug + Send + 'static { | |||
fn accept(&mut self) -> impl Future<Output = io::Result<Option<Conn>>> + Send; | |||
type Conn: ConnExt; |
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.
This naming is very confusing.
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.
It represents a connection instance, any good advice about the naming?
pub trait TransportEndpoint { | ||
fn get_transport(&self) -> Option<Transport>; | ||
fn has_transport(&self) -> bool; | ||
fn set_transport(&mut self, transport: Transport); |
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.
This trait is also confusing.
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.
This is for log and metrics usage to differ between uds and shmipc
in favour of shmipc