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

Adding an Lwt_retry library #1028

Open
shonfeder opened this issue Sep 28, 2024 · 3 comments
Open

Adding an Lwt_retry library #1028

shonfeder opened this issue Sep 28, 2024 · 3 comments

Comments

@shonfeder
Copy link
Contributor

Hello! Thank you for all the work maintaining Lwt, which is a lovely package :)

In our work on the ecosystem infrastructure we have need for retries in many cases, in order to make our systems more robust against transient failures. We have written a tiny library to meet this need, currently called Lwt_retry. It depends only on lwt and lwt.unix and it amounts to a handful of combinators over Lwt.t and Lwt_stream.t.

Would you be interested in including this library into Lwt? If so, I would be happy to open a PR and make any alterations necessary so that it would be a good fit. If not, we will likely try publish this little utility as a standalone, as we need to use it across several code bases.

To get a sense for how we are using this, you can see an example of its usage and the unit tests.

@raphael-proust
Copy link
Collaborator

I've read the interface and had a glance at the example. It's nice. Few things, of varying level of importance:

There's general reticence in adding things into lwt because of the quite strict backwards compatibility requirement which means maintenance burden tends to accrue. Lwt_retry seems small and self-contained enough that we can probably add it to the main Lwt repo. I'd however recommend to add it as a separate package hosted inside the repository. This can help if in the future we need to move it off or more generally make it evolve in some way or another.


The function map_retry seems to only be used for itering for logging. I'm not sure it'd be very useful for much else. Maybe we can leave map but maybe we should instead offer iter? If a user really needs it, they can always rewrite map_retry.


One thing that needs to be made clear in the mli is whether the on_error returns an eager stream (i.e., one where the stream is just a data-structure holding all the retry results, where the computation goes on in the background indefinitely whether the stream is consumed or not) or a lazy stream (i.e., one where the stream is a control-flow mechanism where getting an element triggers a retry).

The documentation as-written seem to suggest the former (“The stream will continue until the computation succeeds or produces a fatal error.” kind of suggests that the stream just keeps goign whatever happens). But the type of with_sleep and n_times suggests the latter.

I wonder if using Lwt_seq.t makes more sense here. It's more clearly a lazy sequence. But OTOH Lwt_seq is not as commonly used… Ideas?

@shonfeder
Copy link
Contributor Author

Thank you for the notes! I will take them into account in preparing a PR to port the library over.

whether the on_error returns an eager stream (i.e., one where the stream is just a data-structure holding all the retry results, where the computation goes on in the background indefinitely whether the stream is consumed or not) or a lazy stream (i.e., one where the stream is a control-flow mechanism where getting an element triggers a retry).

We definitely want the latter IMO, and that is what is intended. The reason being that, iiuc, it should be easy to convert the lazy version into a computation that will run in the background if that's desired, but not possible to go the other way around.

I have no objection to switching to Lwt_seq.t if that is a better fit for the intended behavior.

@raphael-proust
Copy link
Collaborator

Maybe start with a PR closer to the original library rather than switching to Lwt_seq.t (to avoid doing extra-work… because I'm not sure about it).

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

No branches or pull requests

2 participants