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

Rework Interruptible monad #6889

Merged
merged 5 commits into from
Dec 1, 2020
Merged

Conversation

mrmr1993
Copy link
Member

@mrmr1993 mrmr1993 commented Dec 1, 2020

This PR rewrites the interruptible monad to behave correctly. In particular, this changes the semantics around binding so that

let%bind x = (* this gets the interrupt *) in
let%bind y = (* while running this *) in
let%bind z = f y in
(* ... *)

has the same behaviour as

let%bind x = (* this gets the interrupt *) in
let%bind z =
  let%bind y = (* while running this *) in
  f y
in
(* ... *)

This was not the case in either the original version or the tweaked implementation in #6880. Presumably this was causing issues in Integration_test_cloud_engine.Stack_driver_log_engine.pull_subscription_in_background, which calls itself infinitely in this nested-bind style.

This also adds unit tests for these cases, and fixes the existing unit test (which was equivalent to Async.after (Time.Span.of_ms 130.)).

Checklist:

  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them:

Closes #300.

@mrmr1993 mrmr1993 added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label Dec 1, 2020
@mrmr1993 mrmr1993 requested a review from nholland94 December 1, 2020 13:44
@mrmr1993 mrmr1993 requested a review from a team as a code owner December 1, 2020 13:44
*)
type ('a, 's) t

include Monad.S2 with type ('a, 's) t := ('a, 's) t
Copy link
Member

Choose a reason for hiding this comment

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

might be nice to have a comment that says this provides bind, map, and return

let%map () = wait 100. in
assert (!r = 2) )

let%test_unit "interruptions branches do not cancel eachother" =
Copy link
Member

Choose a reason for hiding this comment

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

each other


(** The type of interruptible computations.
[('a, 's) t] represents a computation that produces a value of type ['a],
but which may be interrupted by a signal of type ['s].
Copy link
Member

Choose a reason for hiding this comment

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

might be nice to say what a "signal" is in this context (it's not a Unix signal)

@mergify mergify bot merged commit aadb045 into develop Dec 1, 2020
@mergify mergify bot deleted the feature/interruptible-immediate-halt branch December 1, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch ready-to-merge-into-develop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Give clear semantics and fix implementation of Interruptible monad
3 participants