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

Cyclic Barrier follow up #1417

Merged
merged 16 commits into from
Nov 14, 2020
Merged

Conversation

SystemFw
Copy link
Contributor

Few changes:

  • Changed the state machine to align with Countdown latch, I think it makes for slightly easier logic, see awaitingNow == 0 vs awaiting < capacity - 1
  • Changed the tests to never rely on inspecting the internal state of the barrier
  • Removed awaiting and remaining

@SystemFw SystemFw requested a review from RaasAhsan November 14, 2020 01:34
@SystemFw
Copy link
Contributor Author

@TimWSpence FYI

Copy link

@RaasAhsan RaasAhsan left a comment

Choose a reason for hiding this comment

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

very nice! it's pretty cool how each test assumes the results of previous tests and builds upon them

case State(awaiting, epoch, unblock) =>
val awaitingNow = awaiting - 1

if (awaitingNow == 0)

Choose a reason for hiding this comment

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

nice, this invariant is much clearer

s"$name - raise an exception when constructed with a negative capacity" in real {
val test = IO.defer(constructor(-1)).attempt
test.flatMap { res =>
implicit class Assertions[A](fa: IO[A]) {

Choose a reason for hiding this comment

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

this would be super useful in BaseSpec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it

case Left(e) => e must haveClass[TimeoutException]
})
} yield res
s"$name - await releases all fibers" in real {

Choose a reason for hiding this comment

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

some tests use real and others use ticked, but i can't really tell why :)

Copy link
Contributor Author

@SystemFw SystemFw Nov 14, 2020

Choose a reason for hiding this comment

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

The ones that use ticked need to assert nontermination or cancelation, and ticked let's you do that much more easily. I used real whenever I could though

Choose a reason for hiding this comment

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

nontermination makes sense, but how come cancellation is an issue?

s"$name - await is cancelable" in ticked { implicit ticker =>
      for {	      newBarrier(2).flatMap(_.await).timeoutTo(1.second, IO.unit) must completeAs(())

i thought this was a good candidate for real but i'm probably missing something about how TC works

Choose a reason for hiding this comment

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

oh, is it because cancellation is more or less nondeterministic on the sleep when using real?

Copy link
Contributor Author

@SystemFw SystemFw Nov 14, 2020

Choose a reason for hiding this comment

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

yeah, and ticked is deterministic and fast. With real you tradeoff slow test vs predictable test. Also these tests don't have any of the things that make ticked deadlock-prone (lots of fibers depending on each other) and untrustworthy (real memory barrier things, it's just Ref + Deferred + uncancelable)

@djspiewak djspiewak added the CE 3 label Nov 14, 2020
@djspiewak djspiewak merged commit c7f8046 into typelevel:series/3.x Nov 14, 2020
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