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

Raise and reraise exceptions with Stdlib rather than Lwt #188

Merged
merged 4 commits into from
Sep 17, 2024

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Sep 3, 2024

Lwt's documentation reads:

In most cases, it is better to use failwith s from the standard
library.

and

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.

Prefer to capture backtraces to improve debugability.

See also ocsigen/lwt#1008.

@MisterDA MisterDA force-pushed the lwt-exceptions branch 3 times, most recently from 94d8372 to ae18e09 Compare September 3, 2024 22:03
Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

lib/btrfs_store.ml Show resolved Hide resolved
@MisterDA MisterDA force-pushed the lwt-exceptions branch 3 times, most recently from 1109de1 to 771a198 Compare September 10, 2024 10:58
Lwt's documentation reads:

> In most cases, it is better to use `failwith s` from the standard
> library.

and

> 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.

Prefer to capture backtraces to improve debugability.
Copy link
Contributor Author

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

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

I think this is good to go.

Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looks good. The Unix.ENOENT error from CI is also affecting the main branch, so not due to this PR.

@MisterDA
Copy link
Contributor Author

@shonfeder I can't merge this because of the new rules preventing merges if the CI doesn't pass.

@shonfeder
Copy link
Contributor

@shonfeder I can't merge this because of the new rules preventing merges if the CI doesn't pass.

The system works! :D Tho unfortunately the CI failures here are just #186.

@shonfeder shonfeder merged commit e7d788e into master Sep 17, 2024
12 of 17 checks passed
@shonfeder
Copy link
Contributor

Thanks again for this fix :D

@punchagan punchagan deleted the lwt-exceptions branch September 17, 2024 18:23
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