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

Catch and rethrow exceptions using finally instead of forkFinally #20

Merged
merged 4 commits into from
Jul 31, 2018

Conversation

tswelsh
Copy link
Contributor

@tswelsh tswelsh commented Jul 12, 2018

Using finally after the forkIO means that most exceptions will still be rethrown on the main thread, but forkIO will swallow ThreadKilled exceptions so they will no longer get rethrown. Fixes #19.

@tswelsh
Copy link
Contributor Author

tswelsh commented Jul 31, 2018

I stopped being silly and read what forkFinally actually does. Seems better to follow the original approach and just introduce special handling for ThreadKilled exceptions, so I've adjusted the PR accordingly.

@23Skidoo
Copy link
Collaborator

Please add a changelog note. Otherwise LGTM.

@tswelsh
Copy link
Contributor Author

tswelsh commented Jul 31, 2018

Done - I assumed a minor version bump rather than patch, wasn't terribly sure which it should be. Will change it if you think otherwise.

@23Skidoo
Copy link
Collaborator

Yes, I think this counts as a bugfix.

@23Skidoo 23Skidoo merged commit cc90dee into haskell-github-trust:master Jul 31, 2018
@23Skidoo
Copy link
Collaborator

Merged, thanks! 0.2.4.0 bump was actually fine, the users should be able to guard for the presence of this change with a MIN_VERSION_ macro. I was rather undecided whether to do 0.2.4.0 or go for 0.3.0.0, leaning more towards the former.

I'll make a release tomorrow.

@tswelsh tswelsh deleted the no-thread-killed branch August 1, 2018 06:50
@tswelsh
Copy link
Contributor Author

tswelsh commented Aug 1, 2018

Ah right, sorry for the misunderstanding. Thanks for merging!

@23Skidoo
Copy link
Collaborator

23Skidoo commented Aug 1, 2018

System/Remote/Monitoring/Statsd.hs:150:24: warning: [-Wincomplete-patterns]
    Pattern match(es) are non-exhaustive
    In a case alternative:
        Patterns not matched:
            (Just GHC.IO.Exception.StackOverflow)
            (Just GHC.IO.Exception.HeapOverflow)
            (Just GHC.IO.Exception.UserInterrupt)
    |
150 |             Left e  -> case fromException e of
    |                        ^^^^^^^^^^^^^^^^^^^^^^^...

That's not very nice... I changed the code to rethrow these ones as well.

@23Skidoo
Copy link
Collaborator

23Skidoo commented Aug 1, 2018

Released 0.2.4.0.

@tswelsh
Copy link
Contributor Author

tswelsh commented Aug 1, 2018

Oof yeah that's pretty rubbish, sorry for causing you more work. I just checked how I built it, I wasn't aware that -Wincomplete-patterns isn't in the standard warning set (I have -Wall on pretty much permanently for all work projects). Thanks for the speedy release!

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.

2 participants