haskell-github-trust / ekg-statsd

Flush system metrics to statsd
BSD 3-Clause "New" or "Revised" License
26 stars 23 forks source link

Catch and rethrow exceptions using finally instead of forkFinally #20

Closed tswelsh closed 6 years ago

tswelsh commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Please add a changelog note. Otherwise LGTM.

tswelsh commented 6 years ago

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 commented 6 years ago

Yes, I think this counts as a bugfix.

23Skidoo commented 6 years ago

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 commented 6 years ago

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

23Skidoo commented 6 years ago
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 commented 6 years ago

Released 0.2.4.0.

tswelsh commented 6 years ago

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!