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

Change try-finally to try-with-resources in MetricsFile.write #1505

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

lbergelson
Copy link
Member

Description

There's a weird failure state in CrosscheckFingerPrints where it ends without an exception but writes no metrics. One possible explanation is that the order of exception suppression is suppressing the causal exception and then we're discarding the new exception.

This cleans up the code and changes the order of suppression to match what we want.

* This cleans up the code slightly and should change the behavior of suppressed exceptions
* Now the initial exception thrown within the try block will suppress the closing exception instead of the other way around.
  This may be the cause of an enigmatic silent failure in CrosscheckFingerprints
Copy link
Contributor

@yfarjoun yfarjoun left a comment

Choose a reason for hiding this comment

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

looks great! I wonder how many more of these gems we have….

Copy link
Contributor

@pshapiro4broad pshapiro4broad left a comment

Choose a reason for hiding this comment

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

This is cleaner but I'm skeptical that it fixes your bug. Did you have a test case that you used to verify that this fixed it?

@lbergelson
Copy link
Member Author

@pshapiro4broad I'm also kind of skeptical, but it's the only plausible scenario we've come up with so far.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2020

Codecov Report

Merging #1505 into master will increase coverage by 0.002%.
The diff coverage is 0.000%.

@@               Coverage Diff               @@
##              master     #1505       +/-   ##
===============================================
+ Coverage     69.348%   69.350%   +0.002%     
+ Complexity      8893      8892        -1     
===============================================
  Files            601       601               
  Lines          35436     35432        -4     
  Branches        5900      5899        -1     
===============================================
- Hits           24574     24572        -2     
+ Misses          8533      8530        -3     
- Partials        2329      2330        +1     
Impacted Files Coverage Δ Complexity Δ
...main/java/htsjdk/samtools/metrics/MetricsFile.java 61.265% <0.000%> (+0.954%) 48.000 <0.000> (ø)
...htsjdk/samtools/util/nio/DeleteOnExitPathHook.java 80.952% <0.000%> (-9.524%) 3.000% <0.000%> (-1.000%)

@lbergelson lbergelson merged commit 8a1341d into master Sep 1, 2020
@lbergelson lbergelson deleted the lb_change_to_try_with_resources branch September 1, 2020 20:48
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