Initial pass on catch rewriting; needs review!

[maddie927]

I have a feeling many of these hairy JS functions could be moved to PS, so I want to take a pass at that too before this is accepted. Opening this now to get initial feedback/concerns. This is definitely a scary change, but there are definitely some issues with the current code.

The gist of the problem is that success callback calls shouldn't be wrapped in try/catch because success returns to user code. An Aff/Promise should only ever resolve to 'success' or 'failure'. Wrapping the success callback in a try/catch makes it possible to resolve with 'success' and then later resolve with 'error', possibly causing user code to re-run.

The second issue is that while success shouldn't be caught, calling the Eff callback in makeAff should catch errors. Currently any synchronous exceptions will bypass the Aff error handling and travel up the stack unhandled.

Here is es6-promise's equivalent to makeAff which is nearly the exact same code: https://github.com/stefanpenner/es6-promise/blob/master/lib/es6-promise/-internal.js#L233

This brings me to another thought.. it seems Aff should derive heavily from a good Promise implementation, or maybe even just wrap another implementation. If it wraps native Promises it would require polyfills for some users, but JS people are pretty used to that these days. This would be awesome for FFI as well, as any FFI function that returns Aff would just need the JS side to return a promise-like thing.

[maddie927]

bind is the main place I need to review because that should catch exceptions at some point.

[texastoland]

I reviewed it in person.

Aff should derive [from] Promise [which] would be awesome for FFI

This sounds especially compelling to me!

[maddie927]

Added the start for some tests which pass on this PR but fail in master. I need to add bind tests as well but that is a little more involved since it needs to track how many times each callback gets called for each path (asserting it's always one of the two callbacks but never both and never either more than once).

[cryogenian]

Hello!

I think lack of try/catch in makeAff is expected, because due to signature

makeAff :: ((Error -> Eff e Unit) -> (a -> Eff e Unit) -> Eff e Unit) -> Aff e a

synchronous handlers mustn't throw exceptions.

[garyb]

I'll let @jdegoes chime in with other feedback as I saw he briefly commented in IRC, but I can explain at least one reason why Promise isn't really a suitable basis for Aff: they have bizzarre cancelling semantics.

It should be easy enough to provide a function that converts promises into Aff values though!

[maddie927]

@garyb That makes sense. I didn't consider cancellation.

@cryogenian I disagree. The return type of makeAff is Aff, which supports the concept of failure. That failure path should be the only way for it to fail.

[cryogenian]

@spicydonuts Yes, Aff can fail, but this isn't mandatory to be a part of makeAff. There are throwError and foreign import for it. Perharps Eff callbacks should have (err :: EXCEPTION) effect? Or there should be two makeAff: one with ability to raise exceptions and one without.

[maddie927]

Perhaps not mandatory, but it is a somewhat unexpected edge case, especially for anyone who notices that makeAff looks very similar to new Promise in JS.

I don't have much experience with exception handling in Eff, and zero in Haskell, so I'm curious what the code would look like if those mechanisms were used. Would all makeAff calls be required to wrap their Eff in catchException? Is that even possible when the Eff doesn't declare that it can throw exceptions (even if it does throw them, like request in purescript-node-http). I don't see a clean way to handle those cases. You'd need to write a slightly dishonest FFI function for catching undeclared exceptions.

[maddie927]

Is there a use case for handling asynchronously failing Affs differently than synchronously throwing Affs (especially when they don't declare exception effects, though that seems like potentially a larger issue).

[garyb]

This may be besides the discussed points here, but there is definitely some weirdness with Aff exception handling currently, I recently had to do this: https://github.com/garyb/purescript-quasar/blob/initial/test/src/Test/Main.purs#L79-L91 because the raised errors just seemed to disappear otherwise.

[maddie927]

Ah yeah. I've also seen some weird behavior with forkAff and failing unit tests, causing each failed test to multiply the error output that gets logged.

[maddie927]

This is just my opinion, but it feels like this is the key distinction between Eff and Aff. Eff = synchronous, might throw an exception because it can't do anything else. Aff = asynchronous, errors go through error callbacks. It seems really odd to have Aff simultaneously be synchronous, but only in some cases when un-catchable exceptions are involved. I want to see an Aff return type and trust it to follow that flow in all cases.

[garyb]

I'd tend to agree with that. Even if it's not 100% accurate, as it means we're assuming an EXCEPTION effect everywhere.

[maddie927]

Yeah, it's sort of true both ways you look at it. Doing it basically assumes all Effs have EXCEPTION, but that seems way safer than leaving Aff's semantics fragile. I'd look at it less as an assumption that all Effs can throw and more as a safeguard against the fact that running code at all is dangerous on some level and the point of the Aff type is to bring that complication under control a little bit.

[cryogenian]

That's the case! Most ffi functions should have (err :: EXCEPTION) row in Eff but it's omitted too often.

Why not alter signature to

makeAff 
  :: ((Error -> Eff (err :: EXCEPTION|e) Unit) 
    -> (a -> Eff (err :: EXCEPTION|e) Unit) 
    -> Eff (err :: EXCEPTION|e) Unit) 
  -> Aff e a

?

BTW, this EXCEPTION thing is really weird:

• Aff e a ~ ErrorT (ContT (Eff e a))
• Aff e a ~ ContT (Eff (err :: EXCEPTION|e) a)

I'm not sure what's right approximation here. And, personally, would prefer if all results would be wrapped to Either Error.

Other confusing thing is potentially multiple returned value. If Aff could be used as stream it will be used so (and I've already done this in purescript-routing 😄), if it mustn't -- there should be restriction in type system or at least in runtime.

[maddie927]

You may be right on the signature. Either way though, it's makeAff that's taking an effect as a parameter and encapsulating it in an Aff. You shouldn't be able to break out of that by passing it a questionable (but all too common) Eff.

And streams definitely seem like a separate type/concept. Aff/Promise vs Signal/Behavior/Observable. 90% of the time an Aff that resolves twice is both a mistake and unnoticed by the developer using it (until they start noticing weird bugs or output and spend a bunch of time wondering what's wrong with their own code 🙂).

[garyb]

Aff is currently considered to represent a ContT which has no restriction on "returning" multiple times, it's an expected behaviour when dealing with continuations. But I think there has been discussion other issue/PRs about dropping this idea and treating Aff as one-shot, like promises.

[jdegoes]

But I think there has been discussion other issue/PRs about dropping this idea and treating Aff as one-shot, like promises.

Indeed, now that we have aff-coroutines, there is probably no reason for supporting this style.

An Aff/Promise should only ever resolve to 'success' or 'failure'. Wrapping the success callback in a try/catch makes it possible to resolve with 'success' and then later resolve with 'error', possibly causing user code to re-run.

👍

The second issue is that while success shouldn't be caught, calling the Eff callback in makeAff should catch errors.

👍

Basically, I think it's valid to always assume that Eff code is incorrectly labeled and that it may throw exceptions even if it says it won't. That's just sort of business as usual for Javascript. 😉

it seems Aff should derive heavily from a good Promise implementation, or maybe even just wrap another implementation.

Not while I still have breathe. 😆 💀

Promises/A+ have absolutely awful semantics (especially the intersection of success/error and cancellation, but in lots of places, such as the magical auto-flattening of nested promises), and are impure (once you have a reference to a promise, it's effect is already running).

[jdegoes]

By the way, I haven't had a chance to review this in depth, but will do so when I get a chance.

[maddie927]

Promises/A+ have absolutely awful semantics (especially the intersection of success/error and cancellation, but in lots of places, such as the magical auto-flattening of nested promises), and are impure (once you have a reference to a promise, it's effect is already running).

Definitely in agreement now. 😅
I do like @garyb's suggestion to add functions that convert to/from Promises just for JS interop, but that's probably a separate PR if not a separate module.

[texastoland]

Indeed, now that we have aff-coroutines, there is probably no reason for supporting this style.

This sounds in line chatting with @spicydonuts last night too thanks @jdegoes!

[jdegoes]

I do like @garyb's suggestion to add functions that convert to/from Promises just for JS interop, but that's probably a separate PR if not a separate module.

Yes. I'd even put that in aff-promises just to keep it separate. Happy to host on slamdata org.

By the way, if everyone agrees this PR is consistent with the above goals, well, who am I to argue, you can skip my code review. 😄 My main goal would be to just make sure the behavior is 100% consistent and there are no edge cases.

[texastoland]

Ping. I think this is ready? We have talked about wanting to separate single and multiple resolution (Promise-like versus Observable-like) and lift as much FFI as possible into PureScript but waiting on this one first.

[maddie927]

It'd be helpful for someone with a larger project with tests that uses more of the Aff API to give it a try.

I don't really see the need for splitting up the single/multiple continuation use cases since there are already a few FRP/Observable libs out there. That's actually one of the reasons the current behavior is so surprising for someone not already familiar with ContT; that use case seems to be covered. It'd probably make sense for those Observable implementations to have liftAff or fromAff or toObservable implementations that turn Affs into single-value Observables.

[jdegoes]

@AppShipIt @spicydonuts I'll deal with this today!

Observable-type functionality does not belong in here. There is aff-coroutines which nicely layers streams onto aff. What we could do in here is throw a runtime error if you try to call a makeAff callback more than once. However, I'm not sure that would provide a lot of benefit as there would be no way to catch such an error, so a user would observe it as malfunctioning, which is most likely what will happen if you try to use aff as ContT anyway.

[maddie927]

Yeah.. what about it's tricky.. I think promises just ignore anything after the first call.

[texastoland]

Could something like newtype Settled e = Settled (Eff e Unit) have bind defined in such a way to track none/one/many calls in an effect row for reject and resolve preventing an invalid state from being reached?

[garyb]

We should probably remove the MonadCont instance too if we want to avoid continuation behaviour!

[jdegoes]

@spicydonuts This looks good to me. I haven't tested it with a large code base, but the code looks fine.

Could something like newtype Settled e = Settled (Eff e Unit) have bind defined in such a way to track none/one/many calls in an effect row for reject and resolve preventing an invalid state from being reached?

The "problem" is makeAff, which allows you to pass in a handler which will invoke callbacks arbitrarily many times. Maybe the solution is a handler combinator, something like, justOnce, so you do: makeAff <<< justOnce $ handler. Or maybe even justOnce would take an Eff to run in the event the callbacks are invoked more than once, so you would have some way of dealing with errors (e.g. log to console).

[jdegoes]

OK, unfortunately this needs rebasing now. Also, I am for merging when others are happy. 👍

[maddie927]

Cool, I'll do it soon

[maddie927]

I've rebased this. The only change I wasn't quite sure about was unsafePartial. Is that the right place to add it?

[jdegoes]

@spicydonuts Yes, the dev dependencies will ensure it isn't pulled in for other projects. Looks good to me!