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

Cancellable #785

Merged
merged 5 commits into from Aug 4, 2020
Merged

Cancellable #785

merged 5 commits into from Aug 4, 2020

Conversation

raphael-proust
Copy link
Collaborator

Fixes #782

This PR provides a new cancelable : 'a t -> 'a t function such that cancelable p is a promise p' that is cancelable even if p isn't.

I introduced a new constructor in how_to_cancel. I'm not 100% sure it is necessary but I couldn't find a way to do without it.

@hcarty
Copy link
Contributor

hcarty commented May 18, 2020

The concept makes sense, but it would be nice to give this a clearer name since it doesn't make a promise cancelable. Rather it wraps a promise in a new, cancelable promise. Maybe wrap_in_cancelable?

@raphael-proust
Copy link
Collaborator Author

@hcarty I agree that the name is not really good as is. The type of the function hints that it wraps the promise (rather than modify the promise in place) but the name should also hint at it.

I like wrap_in_cancelable. I tend to like past participles for this kind of things – e.g., how Python has sort (for essentially 'a list -> unit) and sorted ('a list -> 'a list). And that would mirror protected nicely too. But I can't find a good past participle.

Anyway, I'll rename it to something like wrap_in_cancelable if that's the best suggestion I get.

src/core/lwt.ml Outdated Show resolved Hide resolved
test/core/test_lwt.ml Outdated Show resolved Hide resolved
@raphael-proust
Copy link
Collaborator Author

So I actually made it point to the right test (thanks for spotting the error) and there's a bunch of failures where tests raise CamlinternalLazy.Undefined. I'll investigate the feature and maybe I'll have to go without Lazy and reintroduce the Cancel_this_promise_and_propagate_to_one.

@aantron
Copy link
Collaborator

aantron commented Aug 3, 2020

I think, in the worst case, you should be able to define the function using higher-level helpers like on_cancel. I could be wrong. But if that's correct, I suggest to go that route rather than introducing a new internal constructor.

@raphael-proust
Copy link
Collaborator Author

I implemented it with on_cancel for backwards cancellation propagation and on_any for forward resolution propagation.

It doesn't fit with the protected/no_cancel syntactically because of the order the primitives are declared in, but it works.

@aantron aantron merged commit e9babb4 into ocsigen:master Aug 4, 2020
@aantron
Copy link
Collaborator

aantron commented Aug 4, 2020

Thanks!

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.

RFC / Feature request: cancelable
3 participants