8000 Improve safety of `attempt` & `onErrorHandleWith` by oleg-py · Pull Request #566 · monix/monix · GitHub
[go: up one dir, main page]
Skip to content

Improve safety of attempt & onErrorHandleWith - #566

Merged
alexandru merged 7 commits into
monix:masterfrom
oleg-py:fix-handleerrorwith
Jan 29, 2018
Merged

Improve safety of attempt & onErrorHandleWith#566
alexandru merged 7 commits into
monix:masterfrom
oleg-py:fix-handleerrorwith

Conversation

@oleg-py
@oleg-py oleg-py commented Jan 25, 2018 •
edited
Loading
Copy link
Copy Markdown
Collaborator

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 ther 8000 e's nowhere it can go.

@codecov
codecov Bot commented Jan 25, 2018 •
edited
Loading
Copy link
Copy Markdown

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
alexandru commented Jan 25, 2018
Copy link
Copy Markdown
Member

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
alexandru commented Jan 25, 2018
Copy link
Copy Markdown
Member

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
alexandru commented Jan 25, 2018
Copy link
Copy Markdown
Member

FYI — I am fixing completeL.

@Avasil
Avasil commented Jan 25, 2018 •
edited
Loading
Copy link
Copy Markdown
Collaborator

FYI — I am fixing completeL.

Including entire #563?

@oleg-py
oleg-py commented Jan 25, 2018
Copy link
Copy Markdown
Collaborator Author

@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
alexandru commented Jan 25, 2018
Copy link
Copy Markdown
Member

@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
alexandru commented Jan 25, 2018
Copy link
Copy Markdown
Member

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

@alexandru
alexandru commented Jan 25, 2018
Copy link
Copy Markdown
Member

Created the PR - https://github.com/monix/monix/pull/567/files

alexandru
alexandru reviewed Jan 25, 2018
View reviewed changes
@alexandru alexandru left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also see #567

Comment thread monix-tail/shared/src/main/scala/monix/tail/internal/IterantOnError.scala Outdated

/** 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]
@alexandru alexandru Jan 25, 2018
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We usually call this type Attempt.

@oleg-py oleg-py Jan 26, 2018 •
edited
Loading
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

👍 (and a much better name :)

Comment thread monix-tail/shared/src/test/scala/monix/tail/IterantOnErrorSuite.scala Outdated
}
}

test("attempt & onErrorHandleWith should protect against broken continuations") { _ =>
@alexandru alexandru Jan 25, 2018
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@oleg-py oleg-py Jan 26, 2018
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

Comment thread monix-tail/shared/src/main/scala/monix/tail/internal/IterantOnError.scala Outdated
}

def tailGuard(ffa: F[Iterant[F, A]]): F[Iterant[F, A]] =
ffa.handleError(f)
@alexandru alexandru Jan 25, 2018
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 😀

@oleg-py oleg-py Jan 26, 2018
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

@oleg-py oleg-py changed the title WIP: Improve safety of attempt & onErrorHandle 10000 With Improve safety of attempt & onErrorHandleWith Jan 26, 2018
@oleg-py
oleg-py commented Jan 26, 2018
Copy link
Copy Markdown
Collaborator Author

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
alexandru requested changes Jan 27, 2018
View reviewed changes
@alexandru alexandru left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread monix-tail/shared/src/main/scala/monix/tail/internal/IterantOnError.scala Outdated
try fa match {
case Next(a, lt, stop) =>
Next(a, lt.map(loop), stop)
Next(a, tailGuard(stop)(lt.map(loop)), stop)
@alexandru alexandru Jan 27, 2018
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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
alexandru commented Jan 27, 2018
Copy link
Copy Markdown
Member

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

@oleg-py
oleg-py commented Jan 27, 2018
Copy link
Copy Markdown
Collaborator Author

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
@alexandru
alexandru commented Jan 29, 2018
Copy link
Copy Markdown
Member

Thanks @oleg-py

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

Reviewers

@alexandru alexandru alexandru requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@oleg-py @alexandru @Avasil
0