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

Use Stdlib.raise and Lwt.reraise instead of Lwt.fail for better backtraces #36

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

ferminr
Copy link
Contributor

@ferminr ferminr commented Oct 16, 2024

Lwt's documentation reads:

Whenever possible, it is recommended to use raise exn instead, as
raise captures a backtrace, while Lwt.fail does not. If you call
raise exn in a callback that is expected by Lwt to return a
promise, Lwt will automatically wrap exn in a rejected promise,
but the backtrace will have been recorded by the OCaml runtime.

For example, bind's second argument is a callback which returns a
promise. And so it is recommended to use raise in the body of that
callback.

Use Lwt.fail only when you specifically want to create a rejected
promise, to pass to another function, or store in a data structure.

See also mirage/ocaml-cohttp#1079

Note: CI failures on OCaml 5 switches are unrelated to the changes in this PR. That issue is addressed in a separate PR: #35

httpev.ml Outdated
@@ -606,7 +606,7 @@ let handle_lwt ?(single=false) fd k =
let pause = 2. in
log #error "too many open files, disabling accept for %s" (Time.duration_str pause);
Lwt_unix.sleep pause
| `Exn Lwt.Canceled -> log #info "canceling accept loop"; Lwt.fail Lwt.Canceled
| `Exn (Lwt.Canceled as exn) -> log #info "canceling accept loop"; raise exn

Choose a reason for hiding this comment

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

Suggested change
| `Exn (Lwt.Canceled as exn) -> log #info "canceling accept loop"; raise exn
| `Exn (Lwt.Canceled as exn) -> log #info "canceling accept loop"; Lwt.reraise exn

The exn is captured by Exn_lwt.map which should preserve the backtrace of exn. Using reraise here will add to that backtrace rather than starting a new one.


You could also write this function directly with Lwt.try_bind (which makes it more obvious where Canceled came from) but that's somewhat orthogonal:

Lwt.try_bind (fun () -> Lwt_unix.accet fd)
  (fun (fd,addr as peer) -> …)
  (function
    | Unix.Unix_error (Unix.EMFILE,_,_) -> …
    | Lwt.Canceled as exn -> …
    | exn -> …)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. (Force pushed it)

@raphael-proust
Copy link

Apart from the small suggestion above, LGTM

@ferminr ferminr force-pushed the ahrefs/lwt-exceptions branch from d618591 to 824312f Compare October 21, 2024 13:42
@ferminr ferminr merged commit 59465a9 into master Oct 21, 2024
2 of 3 checks passed
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.

2 participants