From d0066b2e10c9c23650253b8a47db4da2ea9573f6 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 29 Jun 2020 00:25:51 +0200 Subject: Move `Server` code into a `server.rs` module --- src/server.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 src/server.rs (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs new file mode 100644 index 0000000..dad6813 --- /dev/null +++ b/src/server.rs @@ -0,0 +1,45 @@ +use std::io; +use std::io::Write; + +use conduit::Handler; + +use crate::request::FastCgiRequest; + + +pub struct Server; + +impl Server { + pub fn start(handler: H) -> io::Result { + fastcgi::run(move |mut raw_request| { + let mut request = FastCgiRequest::new(&mut raw_request).unwrap(); + let response = handler.call(&mut request); + + let mut stdout = raw_request.stdout(); + + let (head, body) = response.unwrap().into_parts(); + + write!( + &mut stdout, + "HTTP/1.1 {} {}\r\n", + head.status.as_str(), + head.status.canonical_reason().unwrap_or("UNKNOWN"), + ); + + for (name, value) in head.headers.iter() { + write!(&mut stdout, "{}: ", name).unwrap(); + stdout.write(value.as_bytes()).unwrap(); + stdout.write(b"\r\n").unwrap(); + } + + stdout.write(b"\r\n").unwrap(); + + match body { + conduit::Body::Static(slice) => stdout.write(slice).map(|_| ()).unwrap(), + conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ()).unwrap(), + conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ()).unwrap(), + }; + }); + + Ok(Server{}) + } +} -- cgit v1.2.3 From 7b4e04adbcc2a7892a45ce537871a2efcb826dd4 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 29 Jun 2020 01:15:00 +0200 Subject: Server::start: Extract `fastcgi::run` request handler to function Make a separate function that can return a result to make it easier to handle errors from the handler. --- src/server.rs | 63 +++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 26 deletions(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index dad6813..297e555 100644 --- a/src/server.rs +++ b/src/server.rs @@ -11,35 +11,46 @@ pub struct Server; impl Server { pub fn start(handler: H) -> io::Result { fastcgi::run(move |mut raw_request| { - let mut request = FastCgiRequest::new(&mut raw_request).unwrap(); - let response = handler.call(&mut request); - - let mut stdout = raw_request.stdout(); - - let (head, body) = response.unwrap().into_parts(); + handle_request(&mut raw_request, &handler); + }); - write!( - &mut stdout, - "HTTP/1.1 {} {}\r\n", - head.status.as_str(), - head.status.canonical_reason().unwrap_or("UNKNOWN"), - ); + Ok(Server{}) + } +} - for (name, value) in head.headers.iter() { - write!(&mut stdout, "{}: ", name).unwrap(); - stdout.write(value.as_bytes()).unwrap(); - stdout.write(b"\r\n").unwrap(); - } +fn handle_request( + mut raw_request: &mut fastcgi::Request, + handler: &H, +) -> Result<(), ()> +where H: Handler + 'static + Sync +{ + let mut request = FastCgiRequest::new(&mut raw_request).unwrap(); + let response = handler.call(&mut request); + + let mut stdout = raw_request.stdout(); + + let (head, body) = response.unwrap().into_parts(); + + write!( + &mut stdout, + "HTTP/1.1 {} {}\r\n", + head.status.as_str(), + head.status.canonical_reason().unwrap_or("UNKNOWN"), + ); + + for (name, value) in head.headers.iter() { + write!(&mut stdout, "{}: ", name).unwrap(); + stdout.write(value.as_bytes()).unwrap(); + stdout.write(b"\r\n").unwrap(); + } - stdout.write(b"\r\n").unwrap(); + stdout.write(b"\r\n").unwrap(); - match body { - conduit::Body::Static(slice) => stdout.write(slice).map(|_| ()).unwrap(), - conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ()).unwrap(), - conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ()).unwrap(), - }; - }); + match body { + conduit::Body::Static(slice) => stdout.write(slice).map(|_| ()).unwrap(), + conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ()).unwrap(), + conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ()).unwrap(), + }; - Ok(Server{}) - } + Ok(()) } -- cgit v1.2.3 From f630e894ea3a4d789e802bf569e1056e46510faf Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 29 Jun 2020 01:42:01 +0200 Subject: server::handle_request: Return errors Add a new error type that we can use to collect and match errors from `handle_request()`. --- src/server.rs | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index 297e555..0dd6d70 100644 --- a/src/server.rs +++ b/src/server.rs @@ -3,7 +3,22 @@ use std::io::Write; use conduit::Handler; -use crate::request::FastCgiRequest; +use snafu::{ResultExt, Snafu}; + +use crate::request; + + +#[derive(Debug, Snafu)] +pub enum Error { + #[snafu(display("{}", source))] + Io { source: io::Error }, + + #[snafu(display("Couldn't build request: {}", source))] + RequestBuilder { source: request::RequestError }, + + #[snafu(display("Couldn't parse response: {}", source))] + ConduitResponse { source: conduit::BoxError }, +} pub struct Server; @@ -21,35 +36,39 @@ impl Server { fn handle_request( mut raw_request: &mut fastcgi::Request, handler: &H, -) -> Result<(), ()> +) -> Result<(), Error> where H: Handler + 'static + Sync { - let mut request = FastCgiRequest::new(&mut raw_request).unwrap(); + let mut request = request::FastCgiRequest::new(&mut raw_request) + .context(RequestBuilder)?; let response = handler.call(&mut request); let mut stdout = raw_request.stdout(); - let (head, body) = response.unwrap().into_parts(); + let (head, body) = response + .context(ConduitResponse)? + .into_parts(); write!( &mut stdout, "HTTP/1.1 {} {}\r\n", head.status.as_str(), head.status.canonical_reason().unwrap_or("UNKNOWN"), - ); + ) + .context(Io)?; for (name, value) in head.headers.iter() { - write!(&mut stdout, "{}: ", name).unwrap(); - stdout.write(value.as_bytes()).unwrap(); - stdout.write(b"\r\n").unwrap(); + write!(&mut stdout, "{}: ", name).context(Io)?; + stdout.write(value.as_bytes()).context(Io)?; + stdout.write(b"\r\n").context(Io)?; } - stdout.write(b"\r\n").unwrap(); + stdout.write(b"\r\n").context(Io)?; match body { - conduit::Body::Static(slice) => stdout.write(slice).map(|_| ()).unwrap(), - conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ()).unwrap(), - conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ()).unwrap(), + conduit::Body::Static(slice) => stdout.write(slice).map(|_| ()).context(Io)?, + conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ()).context(Io)?, + conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ()).context(Io)?, }; Ok(()) -- cgit v1.2.3 From 0d5761aee5af893c90585d375e9bf2545b60d080 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Mon, 29 Jun 2020 01:50:33 +0200 Subject: Rename `request::RequestError` to `request::Error` Now that we have a `request` module, the "Request" part of the name is redundant. --- src/server.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index 0dd6d70..9416d9c 100644 --- a/src/server.rs +++ b/src/server.rs @@ -14,7 +14,7 @@ pub enum Error { Io { source: io::Error }, #[snafu(display("Couldn't build request: {}", source))] - RequestBuilder { source: request::RequestError }, + RequestBuilder { source: request::Error }, #[snafu(display("Couldn't parse response: {}", source))] ConduitResponse { source: conduit::BoxError }, -- cgit v1.2.3 From 6360e9f37a0a65fbc65d9e923db9ef8f5b77ff83 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 4 Jul 2020 02:29:07 +0200 Subject: Server::start: Handle errors with a 500 response If we get errors building the request or Conduit errors, respond with a 500 error. Otherwise, it's a write error, and we should do nothing (ideally log the error), because this means the client closed the connection or there are bigger problems. --- src/server.rs | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index 9416d9c..0f45f5e 100644 --- a/src/server.rs +++ b/src/server.rs @@ -8,6 +8,9 @@ use snafu::{ResultExt, Snafu}; use crate::request; +const HTTP_VERSION: &'static str = "HTTP/1.1"; + + #[derive(Debug, Snafu)] pub enum Error { #[snafu(display("{}", source))] @@ -26,7 +29,19 @@ pub struct Server; impl Server { pub fn start(handler: H) -> io::Result { fastcgi::run(move |mut raw_request| { - handle_request(&mut raw_request, &handler); + match handle_request(&mut raw_request, &handler) { + Ok(_) => (), + + // TODO: log + // Ignore write errors as clients will have closed the + // connection by this point. + Err(Error::Io { .. }) => (), + + Err(Error::RequestBuilder { .. }) => + internal_server_error(&mut raw_request.stdout()), + Err(Error::ConduitResponse { .. }) => + internal_server_error(&mut raw_request.stdout()), + } }); Ok(Server{}) @@ -51,7 +66,8 @@ where H: Handler + 'static + Sync write!( &mut stdout, - "HTTP/1.1 {} {}\r\n", + "{} {} {}\r\n", + HTTP_VERSION, head.status.as_str(), head.status.canonical_reason().unwrap_or("UNKNOWN"), ) @@ -73,3 +89,17 @@ where H: Handler + 'static + Sync Ok(()) } + +fn internal_server_error(mut w: W) { + let code = conduit::StatusCode::INTERNAL_SERVER_ERROR; + + write!( + w, + "{} {} {}\r\n{}\r\n\r\n", + HTTP_VERSION, + code, + code.canonical_reason().unwrap_or_default(), + "Content-Length: 0", + ) + .unwrap_or(()) +} -- cgit v1.2.3 From cdad0271c909c90e800a7e9dd95658b143475d15 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 4 Jul 2020 02:46:01 +0200 Subject: server: Rename `Error::Io` to `Error::WriteError` Make this specifically about errors writing instead of a generic I/O error. Name the variant `WriteError` instead of `Write` because Snafu will generate context selector structs with these names and we already have a `Write` identifier imported. --- src/server.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index 0f45f5e..e3e5d9a 100644 --- a/src/server.rs +++ b/src/server.rs @@ -14,7 +14,7 @@ const HTTP_VERSION: &'static str = "HTTP/1.1"; #[derive(Debug, Snafu)] pub enum Error { #[snafu(display("{}", source))] - Io { source: io::Error }, + WriteError { source: io::Error }, #[snafu(display("Couldn't build request: {}", source))] RequestBuilder { source: request::Error }, @@ -35,7 +35,7 @@ impl Server { // TODO: log // Ignore write errors as clients will have closed the // connection by this point. - Err(Error::Io { .. }) => (), + Err(Error::WriteError { .. }) => (), Err(Error::RequestBuilder { .. }) => internal_server_error(&mut raw_request.stdout()), @@ -71,20 +71,20 @@ where H: Handler + 'static + Sync head.status.as_str(), head.status.canonical_reason().unwrap_or("UNKNOWN"), ) - .context(Io)?; + .context(WriteError)?; for (name, value) in head.headers.iter() { - write!(&mut stdout, "{}: ", name).context(Io)?; - stdout.write(value.as_bytes()).context(Io)?; - stdout.write(b"\r\n").context(Io)?; + write!(&mut stdout, "{}: ", name).context(WriteError)?; + stdout.write(value.as_bytes()).context(WriteError)?; + stdout.write(b"\r\n").context(WriteError)?; } - stdout.write(b"\r\n").context(Io)?; + stdout.write(b"\r\n").context(WriteError)?; match body { - conduit::Body::Static(slice) => stdout.write(slice).map(|_| ()).context(Io)?, - conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ()).context(Io)?, - conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ()).context(Io)?, + conduit::Body::Static(slice) => stdout.write(slice).map(|_| ()).context(WriteError)?, + conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ()).context(WriteError)?, + conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ()).context(WriteError)?, }; Ok(()) -- cgit v1.2.3 From 3914b2189069783e6d538f2d742525284c8951c2 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 4 Jul 2020 02:55:33 +0200 Subject: server: Rename `Error::WriteError` to `Error::Write` and remove context Since we don't actually need any additional context here, we can remove it. Since that allows us to avoid calling `context()` with the `WriteError` context selector, we can then safely rename `WriteError` to `Write`. --- src/server.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index e3e5d9a..2b70c0d 100644 --- a/src/server.rs +++ b/src/server.rs @@ -13,8 +13,8 @@ const HTTP_VERSION: &'static str = "HTTP/1.1"; #[derive(Debug, Snafu)] pub enum Error { - #[snafu(display("{}", source))] - WriteError { source: io::Error }, + #[snafu(context(false))] + Write { source: io::Error }, #[snafu(display("Couldn't build request: {}", source))] RequestBuilder { source: request::Error }, @@ -35,7 +35,7 @@ impl Server { // TODO: log // Ignore write errors as clients will have closed the // connection by this point. - Err(Error::WriteError { .. }) => (), + Err(Error::Write { .. }) => (), Err(Error::RequestBuilder { .. }) => internal_server_error(&mut raw_request.stdout()), @@ -70,21 +70,20 @@ where H: Handler + 'static + Sync HTTP_VERSION, head.status.as_str(), head.status.canonical_reason().unwrap_or("UNKNOWN"), - ) - .context(WriteError)?; + )?; for (name, value) in head.headers.iter() { - write!(&mut stdout, "{}: ", name).context(WriteError)?; - stdout.write(value.as_bytes()).context(WriteError)?; - stdout.write(b"\r\n").context(WriteError)?; + write!(&mut stdout, "{}: ", name)?; + stdout.write(value.as_bytes())?; + stdout.write(b"\r\n")?; } - stdout.write(b"\r\n").context(WriteError)?; + stdout.write(b"\r\n")?; match body { - conduit::Body::Static(slice) => stdout.write(slice).map(|_| ()).context(WriteError)?, - conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ()).context(WriteError)?, - conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ()).context(WriteError)?, + conduit::Body::Static(slice) => stdout.write(slice).map(|_| ())?, + conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ())?, + conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ())?, }; Ok(()) -- cgit v1.2.3 From 4498c8fdf70b67bc65d2d44a32afa901f1594e51 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 4 Jul 2020 03:01:02 +0200 Subject: server::handle_request: Wrap lines --- src/server.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index 2b70c0d..9bbba00 100644 --- a/src/server.rs +++ b/src/server.rs @@ -81,9 +81,12 @@ where H: Handler + 'static + Sync stdout.write(b"\r\n")?; match body { - conduit::Body::Static(slice) => stdout.write(slice).map(|_| ())?, - conduit::Body::Owned(vec) => stdout.write(&vec).map(|_| ())?, - conduit::Body::File(mut file) => io::copy(&mut file, &mut stdout).map(|_| ())?, + conduit::Body::Static(slice) => + stdout.write(slice).map(|_| ())?, + conduit::Body::Owned(vec) => + stdout.write(&vec).map(|_| ())?, + conduit::Body::File(mut file) => + io::copy(&mut file, &mut stdout).map(|_| ())?, }; Ok(()) -- cgit v1.2.3 From ab8b50d5b98ab071c5f70d929f1ca76f3faf71ca Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 4 Jul 2020 03:28:02 +0200 Subject: server: Log errors Provide a mechanism to get detailed error messages when 500 internal server errors happen. We don't want to expose any detailed error information to clients, but it would be useful for administrators to know the cause of any errors. --- src/server.rs | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index 9bbba00..22bf244 100644 --- a/src/server.rs +++ b/src/server.rs @@ -3,6 +3,8 @@ use std::io::Write; use conduit::Handler; +use log::error; + use snafu::{ResultExt, Snafu}; use crate::request; @@ -31,16 +33,22 @@ impl Server { fastcgi::run(move |mut raw_request| { match handle_request(&mut raw_request, &handler) { Ok(_) => (), - - // TODO: log - // Ignore write errors as clients will have closed the - // connection by this point. - Err(Error::Write { .. }) => (), - - Err(Error::RequestBuilder { .. }) => - internal_server_error(&mut raw_request.stdout()), - Err(Error::ConduitResponse { .. }) => - internal_server_error(&mut raw_request.stdout()), + Err(e) => match e { + // Ignore write errors as clients will have closed the + // connection by this point. + Error::Write { .. } => error!("Write error: {}", e), + + Error::RequestBuilder { .. } => { + error!("Unable to build request: {}", e); + + internal_server_error(&mut raw_request.stdout()) + }, + Error::ConduitResponse { .. } => { + error!("Error getting response: {}", e); + + internal_server_error(&mut raw_request.stdout()) + }, + } } }); @@ -95,13 +103,15 @@ where H: Handler + 'static + Sync fn internal_server_error(mut w: W) { let code = conduit::StatusCode::INTERNAL_SERVER_ERROR; - write!( + match write!( w, "{} {} {}\r\n{}\r\n\r\n", HTTP_VERSION, code, code.canonical_reason().unwrap_or_default(), "Content-Length: 0", - ) - .unwrap_or(()) + ) { + Ok(_) => (), + Err(e) => error!("Write error: {}", e), + } } -- cgit v1.2.3 From d125b25f4245df3d0eb80aa726c2a7039945f3de Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Sat, 4 Jul 2020 03:44:48 +0200 Subject: server::internal_server_error: Change `match` to `unwrap_or_else` Reduce a few lines. --- src/server.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'src/server.rs') diff --git a/src/server.rs b/src/server.rs index 22bf244..948a470 100644 --- a/src/server.rs +++ b/src/server.rs @@ -103,15 +103,13 @@ where H: Handler + 'static + Sync fn internal_server_error(mut w: W) { let code = conduit::StatusCode::INTERNAL_SERVER_ERROR; - match write!( + write!( w, "{} {} {}\r\n{}\r\n\r\n", HTTP_VERSION, code, code.canonical_reason().unwrap_or_default(), "Content-Length: 0", - ) { - Ok(_) => (), - Err(e) => error!("Write error: {}", e), - } + ) + .unwrap_or_else(|e| error!("Write error: {}", e)) } -- cgit v1.2.3