Add timeout capability to Future implementation of FetchMonadError #127

justinhj · 2017-07-18T05:11:13Z

The attached PR adds timeout capability to FetchMonadError[Future]

I also modified Query.Sync to return a completed future which I think is the intent rather than run it in a Future

All tests pass but I didn't yet create tests for the timeout, I can do so if the PR looks like something you might want

Only thing I'm not sure about is whether the use of synchronize is needed. I'm not sure but I think Promise may be safe to use from multiple threads because it uses an AtomicReference but didn't find any confirmation of that.

justinhj · 2017-07-18T16:02:45Z

Removed synchronization on Promise and simplified the PR

justinhj · 2017-07-23T03:44:01Z

Have added tests for the two cases of a future failing because it timed out and succeeding because it finished before the timeout occurred.

peterneyens · 2017-08-01T17:00:12Z

shared/src/test/scala/FutureTimeoutTests.scala

+    override def name = "ArticleFuture"
+    override def fetchOne(id: ArticleId): Query[Option[Article]] =
+      Query.async((ok, fail) => {
+        Thread.sleep(delay.toMillis)


Thread.sleep doesn't work on ScalaJS, which is the reason that Travis is complaining.

peterneyens · 2017-08-01T17:02:24Z

shared/src/main/scala/implicits.scala

-        case Async(ac, timeout) => {
+
+        case Sync(e) =>
+          Future.successful({e.value})


Evaluating e could potentially throw an exception and those aren't caught by Future.successful.

peterneyens · 2017-08-01T17:07:10Z

shared/src/test/scala/FutureTimeoutTests.scala

+
+    implicit val dsWillTimeout = ConfigurableTimeoutDatasource(250 milliseconds, 750 milliseconds)
+
+    def article(id: Int): Fetch[Article] = Fetch(ArticleId(id))


If you added an implicit DataSource[ArticleId, Article] you wouldn't need to implement this method in all the tests.

def article(id: Int)(implicit DS: DataSource[ArticeId, Article]): Fetch[Article] = Fetch(ArticleId(id)) // or Fetch(ArticleId(id))(DS)

peterneyens · 2017-08-01T17:26:22Z

shared/src/test/scala/FutureTimeoutTests.scala

+import scala.concurrent.duration._
+import org.scalatest._
+import cats.data.NonEmptyList
+import scala.language.postfixOps


You shouldn't need this as we have "-language:postfixOps" as a scalacOption.

peterneyens · 2017-08-03T10:04:01Z

shared/src/test/scala/FutureTimeoutTests.scala

+    val expTime = System.nanoTime + ms*1000000
+    while(System.nanoTime < expTime) {}
+    println("sleep finished")
+  }


I am not sure if we can actually test the time out in ScalaJS.

I think that if you block like this, you block everything in JS, so the TimerTask cannot finish the promise with a TimeoutExecption (which is happening in the failing test on Travis).

We could revert back to using Thread.sleep and add this as a JVM only test, by moving it to jvm/src/test/scala, what do you think?

Yeah I think that makes sense. I've been trying to think of a way to isolate the timeout behaviour in a way that can be demonstrated to work with scala js but haven't come up with a way yet.

justinhj · 2017-08-03T20:48:41Z

I've updated the PR so that the tests that require non-blocking sleep are now in the JVM module. Seems travis build is failing but this is due to a timeout building the docs

peterneyens · 2017-08-03T21:49:28Z

shared/src/main/scala/implicits.scala

+              timer.schedule(timerTask, timeout.toMillis)
+
+              // Execute the user's action
+              ec.execute(() => {


This conversion (and the one below) to Runnable only works in 2.12 I think which added support for Java 8 SAMs.

Oops my bad. IntelliJ suggested it and it seemed like a good idea. I'll change it back

justinhj · 2017-08-04T18:32:01Z

I think everything looks good now, just getting a timeout with Travis building the docs

peterneyens · 2017-08-04T22:20:34Z

There definitely is something strange going on when combining Timer and tut.

Commenting out implicits.timer (and as a consequence timer.schedule(...)) makes tut run fine.

A minimal tut file which makes tut hang (notice that we aren't even using the time out here, since we are using Query.sync):

// ```tut
import scala.concurrent.{Await, Future}
import scala.concurrent.duration.Duration
import scala.concurrent.ExecutionContext.Implicits.global

import fetch.{FetchMonadError, Query}
import fetch.implicits._

val query: Query[Option[Int]] = Query.sync { Some(1) }

val future: Future[Option[Int]] = FetchMonadError[Future].runQuery(query)
Await.result(future, Duration.Inf)
// ```

If I change the Sync from Future(...) to the line below, tut runs fine for the minimal file, not for the complete docs.md file though ...

case Sync(e) =>
  cats.ApplicativeError[Future, Throwable].catchNonFatalEval(e)

It seems like I need to do some more investigating to see what is going on ...

justinhj · 2017-08-04T23:28:43Z

Ah timer is supposed to run on a daemon thread and if it doesn't then it can stop a program from terminating. I'll check the code and see if I'm doing that correctly

codecov-io · 2017-08-05T16:48:39Z

Codecov Report

Merging #127 into master will increase coverage by 0.43%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
+ Coverage      78%   78.43%   +0.43%     
==========================================
  Files          14       14              
  Lines         300      306       +6     
  Branches        2        2              
==========================================
+ Hits          234      240       +6     
  Misses         66       66

Impacted Files	Coverage Δ
shared/src/main/scala/implicits.scala	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae93d41...af1f8d9. Read the comment docs.

justinhj · 2017-08-05T19:36:26Z

I've updated so Timer is on a daemon thread and also created lazily so it is not initialized by programs that don't use it

peterneyens · 2017-08-07T09:57:27Z

shared/src/main/scala/implicits.scala

-      implicit ec: ExecutionContext
-  ): FetchMonadError[Future] =
+  // Shared Timer object to schedule timeouts
+  lazy val timer: Timer = new Timer(true)


Should we make this private[fetch] ?

peterneyens · 2017-08-07T10:06:21Z

jvm/src/test/scala/FutureTimeoutTests.scala

+
+  case class ArticleId(id: Int)
+  case class Article(id: Int, content: String) {
+    def author: Int = id + 1


I seems this author method isn't used, so we can drop it, right?

Remove unused author ID from test Rename test variable to be more accurate in the output

peterneyens · 2017-08-15T09:06:40Z

shared/src/main/scala/implicits.scala

+            case _ =>
+
+              // Execute the user's action
+              ec.execute(new Runnable {


Nitpicking, we can create this Runnable once before the timeout match { ... }.
Even more nitpicking, we can lose the curly braces around ac(p.trySuccess, p.tryFailure).

Done, maybe there's a better naming for the runnable than 'runnable', suggestions welcome

peterneyens · 2017-08-15T09:06:50Z

shared/src/main/scala/implicits.scala

-      implicit ec: ExecutionContext
-  ): FetchMonadError[Future] =
+  // Shared Timer object to schedule timeouts
+  private[fetch] lazy val timer: Timer = new Timer(true)


We should probably name the daimon thread as well, something like "fetch-future-timeout-daemon" ?

yeah good idea, will help avoid confusion when people look at running threads and don't know what it's for

peterneyens

Thanks @justinhj!

justinhj and others added 2 commits July 17, 2017 22:04

Add timeout capability to Future implementation of FetchMonadError

08e513b

Remove sync and simplify

3e32459

peterneyens self-requested a review July 18, 2017 16:31

justinhj and others added 2 commits July 22, 2017 09:12

add future timeout

23e3511

add tests

797b75a

add test for inifite timeout

dd3cb51

peterneyens reviewed Aug 1, 2017

View reviewed changes

justinhj added 3 commits August 2, 2017 22:13

some changes from review

af4e6dd

backout future.successful change

80ed7d7

tidy up

4f27367

peterneyens reviewed Aug 3, 2017

View reviewed changes

Move future timeout test into JVM module as it cannot run on scala.js

17d0ae9

peterneyens reviewed Aug 3, 2017

View reviewed changes

raulraja mentioned this pull request Aug 3, 2017

Add twitter future support #128

Merged

justinhj added 2 commits August 3, 2017 21:11

Backout java8 only SAM simplification

848a8c4

Put tests in correct folder

a63cb06

Timer is initialized lazily and as a deamon thread

a79af37

peterneyens reviewed Aug 7, 2017

View reviewed changes

Make Timer used in FetchMonadError[Future] private to fetch package

59206ad

Remove unused author ID from test Rename test variable to be more accurate in the output

peterneyens reviewed Aug 15, 2017

View reviewed changes

Add Peters suggestions

af1f8d9

peterneyens approved these changes Aug 15, 2017

View reviewed changes

peterneyens merged commit b1207c1 into xebia-functional:master Aug 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeout capability to Future implementation of FetchMonadError #127

Add timeout capability to Future implementation of FetchMonadError #127

justinhj commented Jul 18, 2017

justinhj commented Jul 18, 2017

justinhj commented Jul 23, 2017

peterneyens Aug 1, 2017

peterneyens Aug 1, 2017

peterneyens Aug 1, 2017

peterneyens Aug 1, 2017

peterneyens Aug 3, 2017

justinhj Aug 3, 2017

justinhj commented Aug 3, 2017

peterneyens Aug 3, 2017

justinhj Aug 3, 2017

justinhj commented Aug 4, 2017

peterneyens commented Aug 4, 2017

justinhj commented Aug 4, 2017

codecov-io commented Aug 5, 2017 •

edited

Loading

justinhj commented Aug 5, 2017

peterneyens Aug 7, 2017

peterneyens Aug 7, 2017

peterneyens Aug 15, 2017

justinhj Aug 15, 2017

peterneyens Aug 15, 2017

justinhj Aug 15, 2017

peterneyens left a comment


		implicit val dsWillTimeout = ConfigurableTimeoutDatasource(250 milliseconds, 750 milliseconds)

		def article(id: Int): Fetch[Article] = Fetch(ArticleId(id))

Add timeout capability to Future implementation of FetchMonadError #127

Add timeout capability to Future implementation of FetchMonadError #127

Conversation

justinhj commented Jul 18, 2017

justinhj commented Jul 18, 2017

justinhj commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinhj commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinhj commented Aug 4, 2017

peterneyens commented Aug 4, 2017

justinhj commented Aug 4, 2017

codecov-io commented Aug 5, 2017 • edited Loading

Codecov Report

justinhj commented Aug 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterneyens left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 5, 2017 •

edited

Loading