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

Implement async, document Instance methods #23

Merged
merged 20 commits into from Jul 28, 2016

Conversation

Projects
None yet
3 participants
@smithjessk
Collaborator

smithjessk commented Jun 22, 2016

  • Add a new subproject, coroutines-extra. This subproject will contain future utilities for Coroutines such as enumerators and continuations
  • Implement Async/Await. This implementation was inspired by the one present in the examples directory.
  • Document more methods on Coroutine.Instance

If you'd prefer, I can split this PR into one for the implementation and one for the documentation.

@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Jun 26, 2016

Member

Nice work - looks good, and I left a few comments to address.

Member

axel22 commented Jun 26, 2016

Nice work - looks good, and I left a few comments to address.

@smithjessk

This comment has been minimized.

Show comment
Hide comment
@smithjessk

smithjessk Jun 28, 2016

Collaborator

I believe that I addressed all of the remaining line comments in 124c3a1. If I missed something, please let me know!

Also, what do you think of the new implementations of async and await? I didn't integrate this comment because I was a bit confused about what you meant.

Collaborator

smithjessk commented Jun 28, 2016

I believe that I addressed all of the remaining line comments in 124c3a1. If I missed something, please let me know!

Also, what do you think of the new implementations of async and await? I didn't integrate this comment because I was a bit confused about what you meant.

@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Jul 4, 2016

Member

You should also add some ScalaMeter benchmarks. You can do that in a follow-up PR.

Member

axel22 commented Jul 4, 2016

You should also add some ScalaMeter benchmarks. You can do that in a follow-up PR.

smithjessk added some commits Jul 8, 2016

Optimize Async implementation
If the future completed, we don't have to create a callback via `onComplete`. Instead, we can directly invoke `loop`.
@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Jul 9, 2016

Member

You can use a doc-comment here.

You can use a doc-comment here.

@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Jul 9, 2016

Member

You need unit tests for this.

You need unit tests for this.

smithjessk added some commits Jul 9, 2016

Stylize more documentation.
Note that this change was meant to be included in the previous commit, fd8712c.
@smithjessk

This comment has been minimized.

Show comment
Hide comment
@smithjessk

smithjessk Jul 10, 2016

Collaborator

I separated the async/await tests back into two files. Also, I will later create a new PR for #26.

Collaborator

smithjessk commented Jul 10, 2016

I separated the async/await tests back into two files. Also, I will later create a new PR for #26.

@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Jul 12, 2016

Member

ok, just give me a PTAL when I need to take a look

Member

axel22 commented Jul 12, 2016

ok, just give me a PTAL when I need to take a look

@smithjessk

This comment has been minimized.

Show comment
Hide comment
@smithjessk

smithjessk Jul 12, 2016

Collaborator

PTAL :)

Collaborator

smithjessk commented Jul 12, 2016

PTAL :)

awaitedFuture.value match {
case Some(Success(result)) =>
loop()
case Some(Failure(exception)) =>

This comment has been minimized.

@axel22

axel22 Jul 22, 2016

Member

Why do you fail the promise at this point? What if a try-catch block wraps the await call? You should just resume and let the await throw the exception.

Can you please add a unit test for this (await call that should throw an exception, which is caught by the enclosing try)?

@axel22

axel22 Jul 22, 2016

Member

Why do you fail the promise at this point? What if a try-catch block wraps the await call? You should just resume and let the await throw the exception.

Can you please add a unit test for this (await call that should throw an exception, which is caught by the enclosing try)?

This comment has been minimized.

@smithjessk

smithjessk Jul 26, 2016

Collaborator

I think I'm misinterpreting the test you're describing because, as of 7fa7713, this passes:

test("await should bubble up exceptions") {
  def thrower() = {
    throw new TestException
    Future(1)
  }

  var exceptionFound = false
  val future = async {
    try {
      await(thrower())
      ()
    } catch {
      case _: TestException => exceptionFound = true
    }
  }
  val r = Await.result(future, 1 seconds)
  assert(exceptionFound)
}

Note that the () was added to the end of the try body to make the compiler happy.

@smithjessk

smithjessk Jul 26, 2016

Collaborator

I think I'm misinterpreting the test you're describing because, as of 7fa7713, this passes:

test("await should bubble up exceptions") {
  def thrower() = {
    throw new TestException
    Future(1)
  }

  var exceptionFound = false
  val future = async {
    try {
      await(thrower())
      ()
    } catch {
      case _: TestException => exceptionFound = true
    }
  }
  val r = Await.result(future, 1 seconds)
  assert(exceptionFound)
}

Note that the () was added to the end of the try body to make the compiler happy.

This comment has been minimized.

@axel22

axel22 Jul 27, 2016

Member

You are misinterpreting what that test does.

The exception you see happens before any failed future object even gets created, and thus, before await even gets a chance to run.

Instead, thrower should return a failed future object, not throw an exception. This is the test you want:



  test("await should bubble up exceptions from failed futures") {
    def failer(): Future[Int] = {
      Future.failed(new TestException)
    }

    var exceptionFound = false
    val future = async {
      try {
        await(failer())
        ()
      } catch {
        case _: TestException => exceptionFound = true
      }
    }
    val r = Await.result(future, 1 seconds)
    assert(exceptionFound)
  }

As a side-note, I checked out this pull request (PR 23) locally, and this test is not present in the codebase.

@axel22

axel22 Jul 27, 2016

Member

You are misinterpreting what that test does.

The exception you see happens before any failed future object even gets created, and thus, before await even gets a chance to run.

Instead, thrower should return a failed future object, not throw an exception. This is the test you want:



  test("await should bubble up exceptions from failed futures") {
    def failer(): Future[Int] = {
      Future.failed(new TestException)
    }

    var exceptionFound = false
    val future = async {
      try {
        await(failer())
        ()
      } catch {
        case _: TestException => exceptionFound = true
      }
    }
    val r = Await.result(future, 1 seconds)
    assert(exceptionFound)
  }

As a side-note, I checked out this pull request (PR 23) locally, and this test is not present in the codebase.

This comment has been minimized.

@axel22

axel22 Jul 27, 2016

Member

I will merge your PR in, and fix this.

@axel22

axel22 Jul 27, 2016

Member

I will merge your PR in, and fix this.

case Success(result) =>
loop()
case Failure(exception) =>
p.failure(exception)

This comment has been minimized.

@axel22

axel22 Jul 22, 2016

Member

Same here.

@axel22

axel22 Jul 22, 2016

Member

Same here.

@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Jul 22, 2016

Member

This almost looks ready, but it looks like you have a bug in the implementation (regarding exceptions in await). Please fix it and add the missing unit tests.

Member

axel22 commented Jul 22, 2016

This almost looks ready, but it looks like you have a bug in the implementation (regarding exceptions in await). Please fix it and add the missing unit tests.

@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Jul 27, 2016

Member

This will get merged automatically in a couple of hours. You will see an additional commit I made to fix the issue I was talking about. Please study the commit carefully to understand what the error was.

Member

axel22 commented Jul 27, 2016

This will get merged automatically in a couple of hours. You will see an additional commit I made to fix the issue I was talking about. Please study the commit carefully to understand what the error was.

@storm-enroute-bot storm-enroute-bot merged commit 06e8224 into storm-enroute:master Jul 28, 2016

2 checks passed

Drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment