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

Generalized exception handling #137

Open
natefaubion opened this issue Jan 26, 2018 · 13 comments
Open

Generalized exception handling #137

natefaubion opened this issue Jan 26, 2018 · 13 comments

Comments

@natefaubion
Copy link
Collaborator

Right now, exceptions in Aff use the underlying Error type of Eff, which is represented by native JS exceptions. There's quite a bit of machinery under the hood for propagating these exceptions in an async manner for very little utility. In my use of Aff, I've found true exceptions to be rare, and because they are largely opaque on the PureScript-side (which I think is a good thing) there's not much to do by catching them. Since the machinery is already there, we should think about generalizing exceptions in Aff, rather than fixing it to Error. That is:

data Aff e a
data Fiber e a

instance monadThrowAff :: MonadThrow e (Aff e)
instance monadErrorAff :: MonadError e (Aff e)
instance bifunctorAff :: Bifunctor Aff

How is this different than just using something like ExceptT? One, it is more efficient. Aff already has to propagate Eithers for results and error, so we already bake in ExceptT handling under the hood. Since we are already paying the cost of it, and not getting that much utility out of the current formulation, we might as well take advantage of it. Otherwise, it's largely a convenience. I don't think there's anything that can't be expressed by using an additional ExceptT layer.

What does this mean for the implementation? For that, I should clarify the three channels we use to propagate async state:

  • The result channel (part of step in the source code). This is just the normal happy path of Aff evaluation.
  • The error channel (fail in the source code). This is used to propagate recoverable exceptions like with throwError.
  • The interrupt channel (interrupt in the source code). This is an irrecoverable panic, which is currently only used with killFiber.

I'll also clarify the cases where we implicitly catch exceptions during the source of evaluation:

  • liftEff. All lifted Effs always have exceptions caught regardless of whether it carries an exception effect.
  • makeAff. We only catch the Eff which initiates the async effect.

The Error type is currently used on both the error channel and the interrupt channel. There isn't any part of the implementation that assumes the Error type for the error channel, so the implementation would largely remain as it is. The question is what do we do about catching exceptions? We'll still catch them, but the only channel available to us (for arbitrary Errors) in this case would be the interrupt channel. This implies that an uncaught exception within a lifted Eff would result in a panic of the current Fiber.

Note that you can still propagate Error on the error channel, since it is parametric, but Aff would not implicitly catch and propagate anything on the error channel. A user would need to explicitly use catchException in liftEff or makeAff. This does not limit any existing workflows.

For forked Fibers which have panicked, any observers (through joinFiber) would necessarily need to panic as well. That is, panics are infectious. Since killFiber users the interrupt/panic channel, killing a Fiber would result in its observers also panicking.

What about forked Fibers which have no observers (that is, nobody is waiting for it's result with joinFiber)? Currently, if a Fiber propagates an error or an interrupt, and no one is there to observe it, we rethrow it in a fresh, global stack using setTimeout. This ensures that exceptions are still observable by things like window.onerror or whatever node uses. However, if we have arbitrary user defined exceptions, we can't rethrow what's on the error channel since it isn't necessarily a instance of Error (though we can still rethrow interrupts). There are a couple of possibilities:

We'd likely need to do both, unless we want to require that all Aff contexts have a Supervisor, which would make sense.

/cc @jdegoes @hdgarrood @chexxor

@chexxor
Copy link

chexxor commented Jan 26, 2018

Currently, I'm returning Aff _ (Either e a) in my async functions using Aff.
With this proposal, you think I should rewrite it to Aff e a? This would be following the JavaScript-idiomatic way of using the error-back - it just throws anything it wants down that channel to break early from the function.

tl;dr summary of below is that your proposal is a pragmatic one and hard to disagree with, but I'm curious how app authors should interact with libraries with produce Aff values.

It feels a bit weird to have Aff bake in the ExceptT behavior and therefore be a little less idiomatic than other PureScript code (only in that not everybody goes straight for ExceptT e Eff a, but instead prefers the "more simple" Eff _ (Either e a)).

That said, I find it hard to argue with being pragmatic here - Aff already has special handling to pipe the errors through nested Affs/Fibers. But it's also a bit weird - changing the error channel from its original purpose in Node of enabling throwing exceptions in async code to being a convenient place way to monadically handle expected failures.

Unrecoverable errors aren't worth catching, so, perhaps you're right, the error channel should be used for monadic, general-purpose error-handling within an app. I wonder how this change would/should affect libraries which returns an Aff Error a or Aff LibraryErrors a value, though. I guess I'd want to always convert the possible library error to my app's error type, as I'd want to handle only a few categories of errors in my app, rather than separately handle every library's specific error. Maybe that's for the best, though.

A story about that: One reason I haven't liked Aff's current/previous error channel is that some web/app frameworks take an Aff value I make and run it for me. The problem with that is that it swallowed my error, it didn't force me to think about it. Those errors unexpectedly came from some presumed "type-safe" libraries which threw on the error channel as generic Error and didn't exactly tell me to expect that. This won't resolve that issue, but it would provide better direction to Aff users and more uniform facilities to people who need to manage Aff errors.

@natefaubion
Copy link
Collaborator Author

natefaubion commented Jan 27, 2018

But it's also a bit weird - changing the error channel from its original purpose in Node of enabling throwing exceptions in async code to being a convenient place way to monadically handle expected failures.

In idiomatic JavaScript, you only have exceptions, there's no monadic error handling.

I wonder how this change would/should affect libraries which returns an Aff Error a or Aff LibraryErrors a value, though.

There is no real semantic difference between Aff LibraryErrors a and Aff Void (Either LibraryErrors a). The latter just has another layer of boxing. With the former, you can choose to defer the error handling if you want, whereas with the latter you must handle it immediately.

attempt :: forall e a. Aff e a -> Aff Void (Either e a)
rethrow :: forall e a. Either e a -> Aff e a

Since we can prove the absence of exceptions, the type of launchAff is no longer a lie since you can require Aff Void a. Libraries can then take an Aff Void a to force you to handle all exceptions before taking over, or it can take an error handler to run.

This pretty much necessitates a monomorphic catch for Aff (not unlike we do for Eff currently), because the MonadError formulation does not allow you to change the error type to prove errors have actually been handled. We can obviously still have a MonadError instance, but you miss out on the proof.

catch :: forall e1 e2 a. (e1 -> Aff e2 a) -> Aff e1 a -> Aff e2 a

This is really just a bind over the left type variable. I think that's worth having in an actual abstraction.

@chexxor
Copy link

chexxor commented Jan 27, 2018

In idiomatic JavaScript, you only have exceptions, there's no monadic error handling.

To clarify that, I was thinking of idiomatic async JavaScript, which uses an error-back and a callback. There, calling the error-back short-circuits the remaining calculations which produce the expected value, which feels monadic to me (where each imperative JS line becomes an Eff bind in PS). My point with that comparison was that idiomatic async JS commonly uses the error-back with arbitrary error messages, which is where you're proposing to take Aff. 👍

@chexxor
Copy link

chexxor commented Jan 27, 2018

Oh also, your examples of using Aff Void a are pretty awesome - it would be fun to try using that in real libs and apps to see how much it improves type-safety and error-handling.

@hdgarrood
Copy link
Contributor

I can certainly see the appeal of this but personally I'm not a huge fan; see #136 (comment).

I think attempt :: forall void e a. Aff e a -> Aff void (Either e a) might be more useful, so that you don't need to lmap absurd if you want to unify it with Aff E for some other concrete error E. See https://www.fpcomplete.com/blog/2017/07/to-void-or-to-void.

@natefaubion
Copy link
Collaborator Author

@MonoidMusician brought up a good point in Slack. ExceptT does not integrate with Aff's bracket behavior. If we want a properly behaved bracket with custom exceptions, it has to be built into Aff.

@natefaubion
Copy link
Collaborator Author

natefaubion commented Jan 27, 2018

@hdgarrood I don't think it's quite the same comparing the way IO uses exceptions to the way Aff uses exceptions. PS has all but completely shunned general exceptions except for interop. And as I pointed out above, there's no difference as far has the tediousness of handling errors between ExceptT Aff and Aff with errors baked in.

Edit: Again, there's no reason you can't continue to propagate Error. Aff Error is the same as current Aff for "real work".

@hdgarrood
Copy link
Contributor

Ok, yeah, fair point. I suppose I can just stick to Aff Error where that makes sense without too much difficulty.

@jdegoes
Copy link
Contributor

jdegoes commented Jan 28, 2018

Since the machinery is already there, we should think about generalizing exceptions in Aff, rather than fixing it to Error.

I'm a big fan of this. Not only does it tremendously clarify semantics (automatically deciding many edge cases that were formerly under-constrained), but it enables types to be significantly more precise. I can require an Aff to handle all errors before I receive it (e.g. at the boundary of HTTP); I can lift programs with smaller errors into programs with larger errors; or, if I choose, I can just use Error everywhere and not have to change any of my code.

@akheron
Copy link
Contributor

akheron commented Nov 16, 2018

Hi! What's the status of this? I'd really like to see this happen, is there something I can do to help?

@natefaubion
Copy link
Collaborator Author

There is just quite a bit of work involved. One thing that really needs consideration is a compatibility story. Aff is heavily depended on library, and is quite stable. Having already been through major breaking changes once already, I'd rather not deal with that again if we can avoid it. I could potentially see co-opting purescript-io, and implementing Aff as a newtype over IO Error, with some shims to preserve semantics. I just haven't worked out the details, but that might be a good place to start from.

@danny-andrews
Copy link

This would be a fantastic improvement for type-safety.

The only major drawback I can think of is unifying error types from 3rd-party libraries, and preventing changes in the number and types of the errors returned from being breaking changes necessarily. (Obviously, if you are branching off of a concrete error type in a 3rd-party library, it would be breaking for you if that type was no longer a part of the error return type of a function you are calling. But it wouldn't have to be breaking for people who let the error bubble-up to some top-level error handler.)

That brings up another point, however. Which is that in order to do anything useful with an error in a top-level error handler, you have to know something about it. If Aff-returning functions can return any arbitrary type, there's no way to generically handle them, even if all you want to do is log the error message somewhere. Maybe the Aff error type should be constrained in someway (ex. it must be a part of the Show typeclass) so all errors can be handled generically.

@danny-andrews
Copy link

Maybe polymorphic variants could be useful here. I’m gonna play around with it and see how it goes.

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

No branches or pull requests

6 participants