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

feat(http server): add new builder APIs build_from_tcp and build_from_hyper #719

Merged
merged 16 commits into from
Apr 1, 2022

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Mar 29, 2022

Before the HTTP server configured the socket explictly but this PR changes to use tokio::net::TcpListener similar to what the WsServer does

In addition this PR adds another API for users to provide their own TcpListener, see the inline example I added with socket2.

Example:

   /// use jsonrpsee_http_server::HttpServerBuilder;
   /// use socket2::{Domain, Socket, Type};
   ///
   /// #[tokio::main]
   /// async fn main() {
   ///   let addr = "127.0.0.1:0".parse().unwrap();
   ///   let domain = Domain::for_address(addr);
   ///   let socket = Socket::new(domain, Type::STREAM, None).unwrap();
   ///   socket.set_nodelay(true).unwrap();
   ///   socket.set_reuse_address(true).unwrap();
   ///   socket.set_nonblocking(true).unwrap();
   ///   socket.set_keepalive(true).unwrap();
   ///
   ///   let address = addr.into();
   ///   socket.bind(&address).unwrap();
   ///
   ///   socket.listen(4096).unwrap();
   ///
   ///   let server = HttpServerBuilder::new().build_from_tcp(socket).await.unwrap();
   /// }
   /// ```

@niklasad1 niklasad1 requested a review from a team as a code owner March 29, 2022 11:45
@jsdw
Copy link
Collaborator

jsdw commented Mar 29, 2022

Ooh offhand this looks very neat, I like it! I also like that it removes socket2 as a dependency and leaves it to the user to provide their own socket thing (if I read it right)!

@niklasad1
Copy link
Member Author

niklasad1 commented Mar 29, 2022

yeah, you are right and that is the idea :)

AFAIK, most socket libraries implements Into<StdTcpListener> and hyper has a similar API

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Loooks good to me; I really like letting somebody provide (and configure) their own socket rather than handling it internally :)

@niklasad1 niklasad1 requested a review from maciejhirsz March 31, 2022 10:42
The hyper settings might contradict to settings on the provided
socket, force users of this API to configure that avoid confusion and
unexpected settings.
http-server/src/server.rs Outdated Show resolved Hide resolved
/// let server = HttpServerBuilder::new().build_from_tcp(socket, hyper_cfg).unwrap();
/// }
/// ```
pub fn build_from_tcp(self, listener: impl Into<StdTcpListener>, cfg: HyperTcpConfig) -> Result<Server<M>, Error> {
Copy link
Member Author

@niklasad1 niklasad1 Apr 1, 2022

Choose a reason for hiding this comment

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

@jsdw is this API reasonable?

Basically the only thing I want to ensure is that no settings gets overwritten without informing the users of this API.

It's a bit awkward to configure some settings twice but no way around that....

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aah ok, so the issue is now basically that you can pass in a TcpListener, but you can't configure the Hyper server that is created from it.

One option might be to keep build_from_tcp() as is (which lets you pass in a pre-configured tcp listener but assumes you don't care about how the resulting hyper server is configured), and also have a build_from_hyper() as well (which lets you control everything about the tcp listener and hyper server if you wish). So then you'd have:

  • build -> can't configure socket, can't configure hyper.
  • build_from_tcp -> can configure tcp socket, can't configure hyper.
  • build_from_hyper -> can configure tcp, can configure hyper.

What do you reckon?

Copy link
Member Author

Choose a reason for hiding this comment

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

fair enough, I think your suggestion is less awkward.

@niklasad1 niklasad1 changed the title [http server]: tcp socket configurations api feat(http server): add new build APIs build_from_tcp and build_from_hyper Apr 1, 2022
@niklasad1 niklasad1 changed the title feat(http server): add new build APIs build_from_tcp and build_from_hyper feat(http server): add new builder APIs build_from_tcp and build_from_hyper Apr 1, 2022
@niklasad1 niklasad1 merged commit 34c2fbe into master Apr 1, 2022
@niklasad1 niklasad1 deleted the na-tcp-socket-configurations-api branch April 1, 2022 20:34
@niklasad1 niklasad1 mentioned this pull request Apr 4, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants