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

refactor: Clean up loading of reading of keydata file in LazyKeyManager #1372

Merged
merged 1 commit into from Dec 11, 2018

Conversation

Projects
None yet
4 participants
@sehrope
Copy link
Contributor

commented Dec 11, 2018

The old code does ensure that the RAF is closed though it's a bit difficult to reason about it as it happens in two different places depending on where an exception occurs. I took a peek at it after seeing #1371.

This PR refactors loading of keyfile data in LazyKeyManager a bit so that IO happens in a separate method and there is only one close operation. This also has the positive side effect of removing one of the // NOSONAR tags.

@davecramer

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

works for me

@davecramer

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

BTW, it appears this has been flagged once already hence the NOSONAR comment; but this is admittedly cleaner

@codecov-io

This comment has been minimized.

Copy link

commented Dec 11, 2018

Codecov Report

Merging #1372 into master will increase coverage by 0.03%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master    #1372      +/-   ##
============================================
+ Coverage     68.68%   68.71%   +0.03%     
- Complexity     3892     3894       +2     
============================================
  Files           179      179              
  Lines         16396    16393       -3     
  Branches       2670     2668       -2     
============================================
+ Hits          11261    11264       +3     
+ Misses         3887     3881       -6     
  Partials       1248     1248

@davecramer davecramer merged commit 9b45e70 into pgjdbc:master Dec 11, 2018

2 checks passed

codecov/project 68.71% (+0.03%) compared to bac4bc1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
raf.readFully(ret);
return ret;
} finally {
raf.close();

This comment has been minimized.

Copy link
@panchenko

panchenko Dec 11, 2018

ideally a possible IOException should be caught here

This comment has been minimized.

Copy link
@davecramer

davecramer Dec 11, 2018

Member

and log it ?

This comment has been minimized.

Copy link
@sehrope

sehrope Dec 11, 2018

Author Contributor

Trapping IOExceptions on close is an anti-pattern. If there's truly an issue closing the resource then you'd want that to bubble up as well.

For files IO specifically I don't think they ever throw anyway but even if they did you'd want to catch it upstream in the catch block of the caller (which then wraps it in a PSQLException).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.