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

Fix ZStream#debounce interruption issue #3765

Merged
merged 3 commits into from Jun 8, 2020
Merged

Conversation

luis3m
Copy link
Contributor

@luis3m luis3m commented Jun 5, 2020

Relates to #3740

@luis3m luis3m requested a review from iravid as a code owner June 5, 2020 19:19
@luis3m
Copy link
Contributor Author

luis3m commented Jun 5, 2020

Actually, I think only the cause.interrupted condition is needed here. The other two cases require the fiber to finish. Will update in a few minutes.

@luis3m
Copy link
Contributor Author

luis3m commented Jun 5, 2020

Done

@luis3m
Copy link
Contributor Author

luis3m commented Jun 6, 2020

I will change tapError with onInterrupt

@luis3m
Copy link
Contributor Author

luis3m commented Jun 8, 2020

Done

@luis3m
Copy link
Contributor Author

luis3m commented Jun 8, 2020

@iravid this is ready for review

Comment on lines 2737 to 2743
}.onInterrupt(
ref.get.flatMap {
case Previous(fiber) => fiber.interrupt
case Current(fiber) => fiber.interrupt
case _ => ZIO.unit
}
)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for this PR dragging out. It's probably better for this finalizer to be attached to the construction of the Ref; this way the fiber is interrupted even if the stream is interrupted "in-between" pulls. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iravid I think you mean:

Ref.make[State](NotStarted).toManaged {
  _.get.flatMap {
    case Previous(fiber) => fiber.interrupt
    case Current(fiber)  => fiber.interrupt
    case _               => ZIO.unit
  }
}

Don't you?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iravid makes sense, done. Also added a new test case for the fiber interruption.

Copy link
Member

@iravid iravid left a comment

Choose a reason for hiding this comment

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

Thank you @luis3m!

@iravid iravid merged commit 197673d into zio:master Jun 8, 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.

None yet

2 participants