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

Fatal exceptions should still cause resource disposal #173

Closed
argv-minus-one opened this issue Aug 28, 2017 · 7 comments
Closed

Fatal exceptions should still cause resource disposal #173

argv-minus-one opened this issue Aug 28, 2017 · 7 comments

Comments

@argv-minus-one
Copy link
Contributor

argv-minus-one commented Aug 28, 2017

The use of Try and NonFatal in the ARM code is a bad idea. Besides things like StackOverflowError, NonFatal also excludes NonLocalReturnControl, which is used by the compiler to implement non-local return. As a result, a non-local return through ARM will leak file handles.

Please use try…finally (and Throwable#addSuppressed, as appropriate) instead, like Java try-with-resources does.

@pathikrit
Copy link
Owner

Fixed in #159. Try v3.1.0

@argv-minus-one
Copy link
Contributor Author

I was using 3.1.0. This problem is not fixed; scala.util.Try ignores fatal exceptions.

I'm working on a PR to solve this problem for real.

@pathikrit pathikrit reopened this Aug 28, 2017
@pathikrit
Copy link
Owner

A small proof-of-concept of code snippet to illustrate problem would be fine here.

@pathikrit
Copy link
Owner

I am curious - I don't have any NonFatal in the codebase ...

@argv-minus-one
Copy link
Contributor Author

argv-minus-one commented Aug 28, 2017

You do have scala.util.Try in the codebase. Try uses NonFatal.

I'm working on debugging some tests I wrote. One of them should demonstrate the problem with the original code. Stand by.

@argv-minus-one
Copy link
Contributor Author

I have posted PR #175 to address this issue.

pathikrit added a commit that referenced this issue Aug 29, 2017
…n-handling

Managed resource exception handling - fix for #173
@pathikrit
Copy link
Owner

@argv-minus-one : Thanks for the PR. I did some DRYing up of the code. Can you please take a look here: #178

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

No branches or pull requests

2 participants