Skip to content

Stop ignoring exceptions in launchAff #33

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

Merged
merged 3 commits into from
Aug 17, 2015
Merged

Stop ignoring exceptions in launchAff #33

merged 3 commits into from
Aug 17, 2015

Conversation

hdgarrood
Copy link
Contributor

launchAff will now throw exceptions rather than swallowing them. Fixes
issue #32

`launchAff` will now throw exceptions rather than swallowing them. Fixes
issue #32
launchAff :: forall e a. Aff e a -> Eff e Unit
launchAff = runAff (const (pure unit)) (const (pure unit))
-- | are ignored, and if the computation produces an error, it is thrown.
launchAff :: forall e a. Aff e a -> Eff (err :: EXCEPTION | e) Unit
Copy link
Contributor

Choose a reason for hiding this comment

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

The EXCEPTION may be thrown asynchronously, whereas the current signature implies that the returned Eff will throw the exception -- and therefore, that such exceptions can be caught using catchException. The implication is invalid in the general case, only valid for purely synchronous Aff.

Maybe it suffices to amend the comment to, "... and if the computation produces an error, it is thrown, but not necessarily in the current thread of execution.".

Copy link
Contributor

Choose a reason for hiding this comment

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

Originally Aff had something like a LATER effect type which could itself accept a row of effects, but that turned out to have problems with type inference. As a consequence, distinctions between "effects now" and "effects later" can't be captured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, of course, thanks. Shall I just amend the comment then, or do you want to see if you can think of a better way of writing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only other alternative I can see is not adding the EXCEPTION effect type, so as not to give the user the (false) assurance they can catch it. It's fundamentally un-catchable. Maybe that's a sign the user should always have to supply an error handler (and we could provide a global one if it makes sense?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok then - so would we end up with something like launchAff = flip runAff (const (pure unit)) then? What would be the type of the global error handler we might provide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or perhaps we could leave the definition like this, but just change the returned type, to, say Eff (errUncatchable :: EXCEPTION | e) Unit?

Copy link
Contributor

Choose a reason for hiding this comment

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

After mulling this over the past couple days, I think the best option right now is to leave it as is, but add the comment. This way you can still 'catch' a synchronous exception (which may in fact happen, since Aff is not necessarily asynchronous), while the comment will warn you away from assuming that future exceptions can be caught synchronously.

Now what we'd really like is for all main functions to expect Aff and not Eff. 😉

@hdgarrood
Copy link
Contributor Author

Ok, sounds good - I added a comment about asynchronousness and catching exceptions. Although, it has just occurred to me that JavaScript doesn't really have "threads", does it?

@hdgarrood
Copy link
Contributor Author

Ok, how's this?

@jdegoes
Copy link
Contributor

jdegoes commented Aug 17, 2015

Thanks!

jdegoes added a commit that referenced this pull request Aug 17, 2015
Stop ignoring exceptions in `launchAff`
@jdegoes jdegoes merged commit 3faa8a3 into purescript-contrib:master Aug 17, 2015
@hdgarrood hdgarrood deleted the rethrow-in-launch branch August 18, 2015 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants