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

Detect asynchronous exceptions via their types #187 #202

Merged
merged 3 commits into from
Feb 14, 2018

Conversation

snoyberg
Copy link
Contributor

This commit uses the same async-exception detection mechanism as is used
by the safe-exceptions package, via checking if the given exception is
cast to a SomeAsyncException. (On older GHCs without SomeAsyncException,
it contains a hard-coded list of async exception types.) It then ensures
that:

  • Throwing via throwChecked always generates a synchronous exception
  • Catching via catchChecked (et al) never catches an asynchronous
    exception

Unfortunately, I don't currently have a reliable test case to ensure
that this fixes the problems described in #187. Hopefully with this
patch available we can begin testing cabal-install and Stack against the
change and see if it resolves the issues.

This commit uses the same async-exception detection mechanism as is used
by the safe-exceptions package, via checking if the given exception is
cast to a SomeAsyncException. (On older GHCs without SomeAsyncException,
it contains a hard-coded list of async exception types.) It then ensures
that:

* Throwing via throwChecked always generates a synchronous exception
* Catching via catchChecked (et al) never catches an asynchronous
  exception

Unfortunately, I don't currently have a reliable test case to ensure
that this fixes the problems described in haskell#187. Hopefully with this
patch available we can begin testing cabal-install and Stack against the
change and see if it resolves the issues.
@snoyberg
Copy link
Contributor Author

Note that I'm not particularly happy about the fallback here for older GHCs (base < 4.7), but there's not much choice in the matter. The alternative would be dropping support for building hackage-security with older versions of GHC. I'm not sure if that's an option, but it would be more reliable, especially with third-party libraries potentially defining their own async exception types (such as the async package).

@edsko
Copy link
Contributor

edsko commented Feb 13, 2018

LGTM. catchChecked was never intended to catch asynchronous exceptions; indeed, it was intended purely to catch checked exceptions, and asynchronous exceptions never are checked. Thanks for the PR!

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

(I'd suggest we squash the second 2 patches into the first)

@snoyberg
Copy link
Contributor Author

Agreed on squashing, let me know if you want me to do that, or you want to do it at merge time.

@hvr hvr merged commit 5763a92 into haskell:master Feb 14, 2018
@snoyberg
Copy link
Contributor Author

Thanks for merging this and #203 @hvr. We're deciding whether to move ahead with merging commercialhaskell/stack#3865 or wait until these changes are released on Hackage. Do you have an ETA on a Hackage release for these changes?

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

Successfully merging this pull request may close these issues.

4 participants