Skip to content
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

Make serve generic over the listener and IO types #2479

Open
wants to merge 3 commits into
base: david/reduce-serve-duplication
Choose a base branch
from

Conversation

davidpdrsn
Copy link
Member

Based on #2478

Fixes #2474

It's actually not as bad as I had feared so I think we can do this for 0.8.

@davidpdrsn davidpdrsn added the breaking change A PR that makes a breaking change. label Jan 1, 2024
@davidpdrsn davidpdrsn added this to the 0.8 milestone Jan 1, 2024
Comment on lines 28 to 41
/// TODO
pub trait Listener: Send + 'static {
/// The listener's IO type.
type Io: AsyncRead + AsyncWrite + Unpin + Send + 'static;

/// The listener's address type.
type Addr: Send;

/// Accept a new incoming connection to this listener
fn accept(&mut self) -> impl Future<Output = io::Result<(Self::Io, Self::Addr)>> + Send;

/// Returns the local address that this listener is bound to.
fn local_addr(&self) -> io::Result<Self::Addr>;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also use tokio-util's Listener trait however I don't think tokio-util should be in our public API. By writing our own we can also use async fn accept rather than fn poll_accept.

@davidpdrsn davidpdrsn mentioned this pull request Jan 7, 2024
1 task
@plaidfinch
Copy link

Just wanted to add a friendly +1 to these changes — this is precisely a fit for something I'm working on right now, and would make life dramatically easier as compared with the boilerplate currently necessary to do this manually.

@plaidfinch
Copy link

Is any help needed to bring this change over the finish line?

@davidpdrsn
Copy link
Member Author

Is any help needed to bring this change over the finish line?

Nope. Its a breaking change and don't wanna start merging breaking PRs just yet. It is done otherwise.

@anna-is-cute
Copy link

Is any help needed to bring this change over the finish line?

I'm not involved with the project, but seeing as this is a breaking change, it's slotted for 0.8 as far as I can tell, so not sure how soon it will be until this is merged.

As a workaround for myself, I cloned the project and merged this PR specifically into the latest version. If it's viable to use for your situation, you can patch it in via the below snippet. I'm using this in prod with no issues, but YMMV obviously.

# in your Cargo.toml
[patch.crates-io]
axum = { git = "https://github.com/lojewalo/axum.git", branch = "uds-v0.7.3" }

@plaidfinch
Copy link

Thanks for doing that @lojewalo, I'll do that, and I'll report if I run into any sharp edges. Thanks for everyone's work on such an awesome library! 😎

@kamulos
Copy link

kamulos commented Mar 25, 2024

I came here via the issue #2494. If this makes it easier to accept tls connections, could you also provide an example how to do it without the axum-server crate?

I am at the moment a bit frustrated about the slow maintenance of the axum-server crate and would prefer to avoid depending on it.

@elichai
Copy link

elichai commented Apr 4, 2024

Any updates on where this stands with the maintainers? do you want to experiment with different designs or can this be merged as-is?
This also solves for me a case of listening over VSOCK with something a little bit different than the regular TCP syscall-flow

@jplatte
Copy link
Member

jplatte commented Apr 5, 2024

This is a PR by the primary maintainer, approved by the co-maintainer. It's going to land, the question is just when. Which I unfortunately can't give you any details about, you're just going to have to be patient (or use this branch via [patch]).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A PR that makes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants