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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve safety of `attempt` & `onErrorHandleWith` #566

Merged
merged 7 commits into from Jan 29, 2018

Conversation

Projects
None yet
3 participants
@oleg-py
Collaborator

oleg-py commented Jan 25, 2018

Closes #562.

I went ahead and made attempt safer too.

Incorporates tests from #559 comments and my latest work on bracket

TODO:

  • Use earlyStop in guards

@alexandru should earlyStop be called if the evaluation of tail results on error? (I suppose it should).

I noticed some inconsitency that seems to not match my understanding of earlyStop:

private[tail] object IterantCompleteL {
  /**
    * Implementation for `Iterant#completeL`
    */
  final def apply[F[_], A](source: Iterant[F, A])
    (implicit F: Sync[F]): F[Unit] = {

    def loop(self: Iterant[F, A]): F[Unit] = {
      try self match { /* ...evicted... */ } catch {
        case ex if NonFatal(ex) =>
          source.earlyStop *> F.raiseError(ex)
      }
    }

Notice that it calls source.earlyStop. But in your comment for #559 you said:

... use a freaking var to hold the latest earlyStop reference

and in this implementation the latest reference would be self.earlyStop. Most other operators name parameter inside loop source instead of self, shadowing the one that apply accepts, so they use latest earlyStop. Is that a bug?

Another question is the one I raised a number of times already 馃槃 .

Suspend(IO.raiseError(ex1), IO.raiseError(ex2)).onErrorHandleWith { ex =>
   /* is ex == ex1 or ex == e2 ? */
}

I assume per discussion in typelevel/cats-effect#113 we are going to swallow the one in earlyStop (which is akin to release), and there's nowhere it can go.

@codecov

This comment has been minimized.

codecov bot commented Jan 25, 2018

Codecov Report

Merging #566 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #566      +/-   ##
==========================================
+ Coverage   90.56%   90.67%   +0.11%     
==========================================
  Files         369      369              
  Lines        9782     9802      +20     
  Branches     1858     1840      -18     
==========================================
+ Hits         8859     8888      +29     
+ Misses        923      914       -9
@alexandru

This comment has been minimized.

Member

alexandru commented Jan 25, 2018

Notice that it calls source.earlyStop. But in your comment for #559 you said:

That's not an inconsistency. It's a bug 鈽癸笍 as it should call self.earlyStop.

Unfortunately such bugs are easy when using inner functions and it's hard to test for it in this case.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 25, 2018

About this:

Suspend(IO.raiseError(ex1), IO.raiseError(ex2)).onErrorHandleWith { ex =>
   /* is ex == ex1 or ex == e2 ? */
}

We can't swallow the one in onErrorHandleWith because it would probably violate the MonadError laws. onErrorHandleWith behaves like flatMap for errors. When the user returns an F.raiseError from onErrorHandleWith, we have to assume that's what he wants. And if an error gets thrown by the function given to onErrorHandleWith, we have to treat that as if the user returned an F.raiseError.

This is a different situation to bracket, as onErrorHandleWith is the equivalent of catch, not the equivalent of finally.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 25, 2018

FYI 鈥 I am fixing completeL.

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Jan 25, 2018

FYI 鈥 I am fixing completeL.

Including entire #563?

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Jan 25, 2018

@alexandru I don't return raiseError 馃槢. As my Iterant, I have a Suspend of rest and earlyStop that both fail. Which error of two do I pass into onErrorHandleWith?

Fix for completeL will be nice - it should make bracket handle broken cursors

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 25, 2018

@Avasil not the entire issue, if you want you can take care of the others. Was thinking to put into practice my var idea to see if it works.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 25, 2018

@oleg-py that's a tough call 鈥 prefer ex1 (the error from rest), but do what's easy.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 25, 2018

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Jan 25, 2018

Ok, I can take care of the others tomorrow, should be easy if your idea works, I'll do comparison benchmark too

@@ -101,16 +106,20 @@ private[tail] object IterantOnError {
/** Implementation for `Iterant.attempt`. */
def attempt[F[_], A](fa: Iterant[F, A])(implicit F: Sync[F]): Iterant[F, Either[Throwable, A]] = {
type ErrOrA = Either[Throwable, A]

This comment has been minimized.

@alexandru

alexandru Jan 25, 2018

Member

We usually call this type Attempt.

This comment has been minimized.

@oleg-py

oleg-py Jan 26, 2018

Collaborator

馃憤 (and a much better name :)

@@ -83,4 +85,64 @@ object IterantOnErrorSuite extends BaseTestSuite {
}
}
test("attempt & onErrorHandleWith should protect against broken continuations") { _ =>

This comment has been minimized.

@alexandru

alexandru Jan 25, 2018

Member

You probably have repetitive code, but this test should be broken in two, because when it fails in Travis, it would be good to know which of them failed, attempt or onErrorHandleWith.

This comment has been minimized.

@oleg-py

oleg-py Jan 26, 2018

Collaborator

I hope it's okay if I just extract common code to a method? 馃槒

@@ -48,18 +48,23 @@ private[tail] object IterantOnError {
Suspend[F, A](next, stop)
}
def tailGuard(ffa: F[Iterant[F, A]]): F[Iterant[F, A]] =
ffa.handleError(f)

This comment has been minimized.

@alexandru

alexandru Jan 25, 2018

Member

PS: gone ahead and opened a ticket for a transformWith in MonadError, fingers crossed 鈥 typelevel/cats#2161

We can't wait for it though. It will be ages before that happens, if at all 馃榾

This comment has been minimized.

@oleg-py

oleg-py Jan 26, 2018

Collaborator

That won't hurt. In the meantime, I think, users will appreciate correctness anyway 馃槃

@oleg-py oleg-py changed the title from WIP: Improve safety of `attempt` & `onErrorHandleWith` to Improve safety of `attempt` & `onErrorHandleWith` Jan 26, 2018

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Jan 26, 2018

Ok, that should be it. I decided not to use the var trick - it seems to complicate the code with no benefit (as I cannot reduce the number of calls to handleErrorWith).

@alexandru

You made the right decision in not doing the var optimization 鈥 it's very risky, because Iterant is served piecemeal, which means that the effects of such a var can end up exposed to the user.

And even though F[_] values in Iterant are supposed to be side effectful on evaluation, we should abstain from side effects that can take the user by surprise.

def loop(fa: Iterant[F, A]): Iterant[F, A] =
try fa match {
case Next(a, lt, stop) =>
Next(a, lt.map(loop), stop)
Next(a, tailGuard(stop)(lt.map(loop)), stop)

This comment has been minimized.

@alexandru

alexandru Jan 27, 2018

Member

Everything looks good except that I feel this call is overreaching. It should probably not include the .map(loop):

tailGuard(lt, stop).map(loop)

For one this is because it's unnecessary and also because we're avoiding optimization opportunities 鈥 e.g. lt could be a pure value for which the F[_] implementation could bypass error handling.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 27, 2018

@oleg-py is there some channel to reach you? Can we chat on Gitter? 馃榾

@oleg-py

This comment has been minimized.

Collaborator

oleg-py commented Jan 27, 2018

I moved map outside the guard for handleError, but not for attempt since that way types don't align. Also, removed currying, it seems totally redundant now

@alexandru alexandru merged commit 112169f into monix:master Jan 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexandru

This comment has been minimized.

Member

alexandru commented Jan 29, 2018

Thanks @oleg-py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment