Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork98
feat: uds#496
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
feat: uds#496
Uh oh!
There was an error while loading.Please reload this page.
Conversation
semanticdiff-combot commentedNov 6, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Review changes with |
It can be the three types, but let me think a bit about it, maybe we do not need to do this. I will come back to this point soon.
Yes, that method is convoluted and we are aware of that. About this, there was an attempt to extract functionality and improve the code, for example,#377, but unfortunately got stuck. The question is, do you want to do it before UDS or after? |
Im sure there doesn't need to be a
I think it makes sense to do the refactoring together with adding uds support. The uds part isn't that big of a change i think so the changes should still be manageable. |
Honestly, It sounds fine to me. I also think this should allow us to decouple But keep in mind do not disrupt existing functionality and properly advise users to give them a proper path to migrate if the case.
Feel free to go ahead. A prototype of this should be good to see. If you need any help just let me know. |
What about an interface like this below that could convert from pubenumInterface{Address(IpAddr),Path(PathBuf),} |
| http2_server.await?; | ||
| } | ||
| #[cfg(unix)] | ||
| let signals = | ||
| signals::create_signals().with_context(|| "failed to register termination signals")?; | ||
| #[cfg(unix)] | ||
| let handle = signals.handle(); | ||
| #[cfg(unix)] | ||
| handle.close(); | ||
| tcp_listener | ||
| .set_nonblocking(true) | ||
| .with_context(|| "failed to set TCP non-blocking mode")?; | ||
| #[cfg(windows)] | ||
| _cancel_fn(); | ||
| let http1_server = HyperServer::from_tcp(tcp_listener) | ||
| .unwrap() | ||
| .tcp_nodelay(true) | ||
| .serve(router_service); | ||
| server_warn!("termination signal caught, shutting down the server execution"); | ||
| return Ok(()); | ||
| } | ||
| #[cfg(unix)] | ||
| ServerType::Uds(unix_listener) => { | ||
| let addr = unix_listener.listener.local_addr()?; | ||
| let http1_server = HyperServer::builder(unix_listener).serve(router_service); | ||
| let http1_cancel_recv = Arc::new(Mutex::new(_cancel_recv)); | ||
| #[cfg(unix)] | ||
| let http1_cancel_recv = Arc::new(Mutex::new(_cancel_recv)); | ||
| let http1_server = http1_server.with_graceful_shutdown(signals::wait_for_signals( | ||
| signals, | ||
| grace_period, | ||
| http1_cancel_recv, | ||
| )); | ||
| #[cfg(unix)] | ||
| let http1_server = http1_server.with_graceful_shutdown(signals::wait_for_signals( | ||
| signals, | ||
| grace_period, | ||
| http1_cancel_recv, | ||
| )); | ||
| server_info!( | ||
| parent: tracing::info_span!("Server::start_server", ?threads), | ||
| "http1 server is listening at {:?}", | ||
| addr | ||
| ); | ||
| #[cfg(windows)] | ||
| let http1_server = http1_server.with_graceful_shutdown(async move { | ||
| let http1_cancel_recv = if general.windows_service { | ||
| // http1_cancel_recv | ||
| Arc::new(Mutex::new(_cancel_recv)) | ||
| } else { | ||
| // http1_ctrlc_recv | ||
| Arc::new(Mutex::new(Some(receiver))) | ||
| }; | ||
| signals::wait_for_ctrl_c(http1_cancel_recv, grace_period).await; | ||
| }); | ||
| server_info!("press ctrl+c to shut down the server"); | ||
| server_info!( | ||
| parent: tracing::info_span!("Server::start_server", ?addr_str, ?threads), | ||
| "http1 server is listening on http://{}", | ||
| addr_str | ||
| ); | ||
| http1_server.await?; | ||
| server_info!("press ctrl+c to shut down the server"); | ||
| handle.close(); | ||
| #[cfg(unix)] | ||
| http1_server.await?; | ||
| if let Some(path) = addr.as_pathname() { | ||
| tokio::fs::remove_file(path).await?; | ||
| } | ||
| #[cfg(windows)] | ||
| let http1_server_task = tokio::spawn(async move { | ||
| if let Err(err) = http1_server.await { | ||
| tracing::error!("http1 server failed to start up: {:?}", err); | ||
| std::process::exit(1) | ||
| server_warn!("termination signal caught, shutting down the server execution"); | ||
| Ok(()) | ||
| } | ||
| }); | ||
| #[cfg(windows)] | ||
| tokio::try_join!(ctrlc_task, http1_server_task)?; | ||
| #[cfg(windows)] | ||
| _cancel_fn(); | ||
| #[cfg(unix)] | ||
| handle.close(); | ||
| server_warn!("termination signal caught, shutting down the server execution"); | ||
| Ok(()) | ||
| }; |
Check failure
Code scanning / clippy
unneeded return statement Error
Description
This is a very crude proof of concept for implementing unix domain socket support. Currently only uds works and the path is hardcoded to
/tmp/sws.sock. I wanted to have some feedback first, before fully implementing this. Here are a few points i would like to have feedback on / discuss:http2boolean with alisten_type(or similar) String.start_servermethod is a bit too long imo, does it make sense to have seperate start functions for each type? On the other hand there is already some duplicate code in there, and splitting that into functions would make it harder to reuse code across listeners.Related Issue
closes#484
Motivation and Context
See#484
I mainly want this because remembering paths is simpler than remembering ports, and for the performance.
How Has This Been Tested?
Tested functionality with curl, works as expected.
Screenshots (if appropriate):