-
Notifications
You must be signed in to change notification settings - Fork 104
Report handler panics as panics, not client disconnects #1677
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| // Copyright 2026 Oxide Computer Company | ||
|
|
||
| //! Reproduces, interactively, how Dropshot reports a request handler that | ||
| //! panics: what appears in the log, what the request-done DTrace probe | ||
| //! reports, and what the client observes on the wire. | ||
| //! | ||
| //! Run it with the probes compiled in: | ||
| //! | ||
| //! ```text | ||
| //! cargo run --example panic-handler --features usdt-probes | ||
| //! ``` | ||
| //! | ||
| //! then follow the printed instructions: optionally attach dtrace with the | ||
| //! printed one-liner, press Enter, and watch the sequence for a request to | ||
| //! `/panic`. The server stays up afterward for further poking with curl. | ||
| //! | ||
| //! The log goes to stderr at debug level so that every message involved in | ||
| //! the sequence is visible, including debug-level breadcrumbs. | ||
|
|
||
| use dropshot::ApiDescription; | ||
| use dropshot::ConfigLogging; | ||
| use dropshot::ConfigLoggingLevel; | ||
| use dropshot::HttpError; | ||
| use dropshot::HttpResponseOk; | ||
| use dropshot::ProbeRegistration; | ||
| use dropshot::RequestContext; | ||
| use dropshot::ServerBuilder; | ||
| use dropshot::endpoint; | ||
| use tokio::io::AsyncReadExt; | ||
| use tokio::io::AsyncWriteExt; | ||
|
|
||
| #[endpoint { | ||
| method = GET, | ||
| path = "/panic", | ||
| }] | ||
| async fn example_panic( | ||
| _rqctx: RequestContext<()>, | ||
| ) -> Result<HttpResponseOk<u64>, HttpError> { | ||
| panic!("oh no, a panic!"); | ||
| } | ||
|
|
||
| #[endpoint { | ||
| method = GET, | ||
| path = "/ok", | ||
| }] | ||
| async fn example_ok( | ||
| _rqctx: RequestContext<()>, | ||
| ) -> Result<HttpResponseOk<u64>, HttpError> { | ||
| Ok(HttpResponseOk(1)) | ||
| } | ||
|
|
||
| #[tokio::main] | ||
| async fn main() -> Result<(), String> { | ||
| let config_logging = | ||
| ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Debug }; | ||
| let log = config_logging | ||
| .to_logger("panic-handler-example") | ||
| .map_err(|error| format!("failed to create logger: {}", error))?; | ||
|
|
||
| let mut api = ApiDescription::new(); | ||
| api.register(example_panic).unwrap(); | ||
| api.register(example_ok).unwrap(); | ||
|
|
||
| let server = ServerBuilder::new(api, (), log) | ||
| .start() | ||
| .map_err(|error| format!("failed to start server: {}", error))?; | ||
| let addr = server.local_addr(); | ||
| let pid = std::process::id(); | ||
|
|
||
| println!(); | ||
| println!("server: http://{}", addr); | ||
| println!("pid: {}", pid); | ||
| match server.probe_registration() { | ||
| ProbeRegistration::Succeeded => println!("probes: registered"), | ||
| other => println!( | ||
| "probes: NOT registered ({:?}); \ | ||
| rebuild with --features usdt-probes", | ||
| other | ||
| ), | ||
| } | ||
| println!(); | ||
| println!("to watch the request-done probe, run (in another terminal):"); | ||
| println!(); | ||
| println!( | ||
| " dtrace -q -p {} -n 'dropshot$target:::request-done \ | ||
| {{ printf(\"%s\\n\", copyinstr(arg0)); }}'", | ||
| pid | ||
| ); | ||
| println!(); | ||
| println!( | ||
| "press Enter to make a request to /panic \ | ||
| (attach dtrace first if you want the probe) ..." | ||
| ); | ||
| tokio::task::spawn_blocking(|| { | ||
| let mut line = String::new(); | ||
| let _ = std::io::stdin().read_line(&mut line); | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // Make the request over a raw TCP connection so that exactly what the | ||
| // client observes on the wire can be reported. | ||
| let mut stream = tokio::net::TcpStream::connect(addr) | ||
| .await | ||
| .map_err(|error| format!("failed to connect: {}", error))?; | ||
| stream | ||
| .write_all(b"GET /panic HTTP/1.1\r\nhost: example\r\n\r\n") | ||
| .await | ||
| .map_err(|error| format!("failed to send request: {}", error))?; | ||
| let mut buf = Vec::new(); | ||
| let result = stream.read_to_end(&mut buf).await; | ||
| println!(); | ||
| println!("the client's view of GET /panic:"); | ||
| println!(" response bytes received: {}", buf.len()); | ||
| match result { | ||
| Ok(_) => println!(" connection closed (clean EOF), no response"), | ||
| Err(error) => println!(" connection aborted: {}", error), | ||
| } | ||
|
|
||
| println!(); | ||
| println!("server still running; things to try:"); | ||
| println!(" curl -v http://{}/panic", addr); | ||
| println!(" curl -v http://{}/ok", addr); | ||
| println!("^C to exit"); | ||
| server.await | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -306,27 +306,53 @@ pub enum HandlerError { | |
| /// a structured value, so that the internal and external messages of the | ||
| /// error can both be logged. | ||
| Dropshot(HttpError), | ||
| /// The handler panicked while executing. | ||
| /// | ||
| /// The panic payload is carried as a value so that the server can report | ||
| /// the panic -- attributed to the handler, with its message -- and then | ||
| /// resume the unwind, preserving the behavior that a handler panic aborts | ||
| /// the connection without a response. In | ||
| /// [`HandlerTaskMode::Detached`][crate::HandlerTaskMode::Detached], the | ||
| /// payload comes from tokio's `JoinError` (the panic was already caught | ||
| /// at the task boundary); in `CancelOnDisconnect`, it is caught around | ||
| /// the handler call itself. | ||
| Panicked { | ||
| message: String, | ||
| payload: Box<dyn std::any::Any + Send + 'static>, | ||
| }, | ||
| } | ||
|
|
||
| impl HandlerError { | ||
| pub(crate) fn status_code(&self) -> StatusCode { | ||
| match self { | ||
| Self::Handler { rsp, .. } => rsp.status(), | ||
| Self::Dropshot(e) => e.status_code.as_status(), | ||
| // A panic produces no response and therefore has no status code | ||
| // (the request-done DTrace probe reports the sentinel 0, "no | ||
| // response was received"). Reporting any real status here would | ||
| // contradict that. `http_request_handle_wrap` matches `Panicked` | ||
| // before any path that asks for a status code. | ||
| Self::Panicked { message, .. } => { | ||
| unreachable!( | ||
| "a handler panic ({message:?}) has no status code", | ||
| ); | ||
| } | ||
|
Comment on lines
+330
to
+339
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a footgun to me. How about making the method fallible instead and letting callers use |
||
| } | ||
| } | ||
|
|
||
| pub(crate) fn internal_message(&self) -> &String { | ||
| match self { | ||
| Self::Handler { message, .. } => message, | ||
| Self::Dropshot(e) => &e.internal_message, | ||
| Self::Panicked { message, .. } => message, | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn external_message(&self) -> Option<&String> { | ||
| match self { | ||
| Self::Handler { .. } => None, | ||
| Self::Dropshot(e) => Some(&e.external_message), | ||
| Self::Panicked { .. } => None, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -359,6 +385,15 @@ impl HandlerError { | |
| rsp | ||
| } | ||
| Self::Dropshot(e) => e.into_response(request_id), | ||
| // A panic is reported and then resumed by | ||
| // `http_request_handle_wrap`, never converted into a response; | ||
| // see the `HandlerError::Panicked` match arm there. | ||
| Self::Panicked { message, .. } => { | ||
| unreachable!( | ||
| "a handler panic ({message:?}) must be resumed, \ | ||
| not converted into a response", | ||
| ); | ||
| } | ||
|
Comment on lines
+388
to
+396
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, if the caller's never going to call it on this variant, then let's make this return a I wonder if there's a way to make this all compile-time checkable instead. A simple way to do that would be to break this into two variants, Panicked and Handleable, with a separate two-variant enum for Dropshot vs. Handler error. That seems awkward but is there something like that we could do? |
||
| } | ||
| } | ||
| } | ||
|
|
||
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 neat, but I'd be inclined to skip all this and let the user run
curl(orcurl -v) by hand if they want to. That'll give them whatever control they need, will let them run more than one request, etc.It especially doesn't seem worthwhile to write an HTTP string directly on the socket here.