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

Add unsafeRunToTwitterFuture for twitter Future #882

Merged
merged 3 commits into from May 29, 2019

Conversation

yaroot
Copy link
Contributor

@yaroot yaroot commented May 22, 2019

Allow evaluating ZIO to twitter Future. Unfortunately the only place I could add this method is the Runtime[R] because it needs to evaluate Fiber.interrupt when being cancelled

@yaroot yaroot force-pushed the twitter-future branch 3 times, most recently from 4de0e6e to bed5b2e Compare May 22, 2019 06:17
@jdegoes
Copy link
Member

jdegoes commented May 23, 2019

Looks good! Just a couple minor comments on efficiency.

Copy link
Member

@jdegoes jdegoes left a comment

Choose a reason for hiding this comment

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

Which test is the flaky one? Happy to help you with it!

@yaroot
Copy link
Contributor Author

yaroot commented May 24, 2019

propagateInterrupts fail most of the time, basically fiber.interrupt should cancel the underlying future but it doesn't, making the whole thing fail.

https://github.com/scalaz/scalaz-zio/blob/d64faa87c154b522a767568831b6f8cab6468a50/interop-twitter/jvm/src/test/scala/scalaz/zio/interop/TwitterSpec.scala#L50-L57

@jdegoes
Copy link
Member

jdegoes commented May 24, 2019

@yaroot

The logic is not very simple, but it's possible fromTwitterFuture is not propagating the interruption signal.

This would be a more logical implementation to me:

    final def fromTwitterFuture[A](future: Task[Future[A]]): Task[A] =
      Task.uninterruptibleMask(restore =>
        future.flatMap { f =>
          restore(Task.effectAsync { cb =>
            f.respond {
              case Return(a) => cb(Task.succeed(a))
              case Throw(e)  => cb(Task.fail(e))
            }
          }).onInterrupt(UIO(f.raise(new FutureCancelledException)))
        )
      }

@yaroot
Copy link
Contributor Author

yaroot commented May 24, 2019

@jdegoes ah yes, I println debugged it, the promise interruption handler was never called.

@yaroot
Copy link
Contributor Author

yaroot commented May 28, 2019

@jdegoes could you take another look?

Copy link
Member

@jdegoes jdegoes 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 to me. Thank you for your work on this!

@jdegoes jdegoes merged commit 75e4e70 into zio:master May 29, 2019
@jdegoes
Copy link
Member

jdegoes commented May 29, 2019

Congratulations on your first ZIO pull request, and thank you for the contribution!

giphy

@yaroot yaroot deleted the twitter-future branch May 30, 2019 02:32
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