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

Countdown latch for std #1404

Merged
merged 9 commits into from
Nov 18, 2020
Merged

Conversation

TimWSpence
Copy link
Member

No description provided.

Copy link
Contributor

@SystemFw SystemFw 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, small comments here and there

import java.util.concurrent.TimeoutException

class CountDownLatchSpec extends BaseSpec {
sequential
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these sequential?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I copied the header from QueueSpec 😂 I'm not actually sure why it needs sequential either?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I would remove it unless someone else has a reason for it. I vaguely recall one place where it was actually needed, but then I think it spread through copy-paste

Copy link
Member

Choose a reason for hiding this comment

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

Only the laws tests need sequential. Everything else can be in parallel for maximum speeds. :-)

case Awaiting(n, signal) =>
if (n > 1) (Awaiting(n - 1, signal), F.unit) else (Done(), signal.complete(()).void)
case d @ Done() => (d, F.unit)
}.flatten
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make this uncancelable

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good point! Just to check my understanding: if we observed cancellation after modify and before flatten then we would have transitioned to the Done state without ever unblocking the fibers by completing the signal?

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly. You can solve it by adding an onCancel that checks the state yada-yada, but because complete is not a long-lived operation, it's overwhelmingly better to just make the whole thing uncancelable (since modify is already uncancelable anyway)


/**
* Concurrency abstraction that supports semantically blocking
* until n latches are released
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth adding a comment about this being one-shot (in slightly less concise terms :) )

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good call 👍

@djspiewak djspiewak added the CE 3 label Nov 14, 2020
Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Minor comment but otherwise good!

name: String,
constructor: Int => IO[CountDownLatch[IO]]): Fragments = {

s"$name - raise an exception when constructed with negative initial latches" in real {
Copy link
Member

Choose a reason for hiding this comment

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

name should { :-)

@TimWSpence TimWSpence requested a review from djspiewak November 17, 2020 09:13
@djspiewak djspiewak merged commit 5a455ed into typelevel:series/3.x Nov 18, 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