Skip to content
This repository has been archived by the owner on Feb 3, 2018. It is now read-only.

Avoid crash with a lock file #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Avoid crash with a lock file #162

wants to merge 1 commit into from

Conversation

kaneshin
Copy link

@kaneshin kaneshin commented Jan 27, 2017

Possibly crash this code.

@codecov-io
Copy link

codecov-io commented Jan 27, 2017

Codecov Report

Merging #162 into master will increase coverage by 0.09%.

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
+ Coverage   78.88%   78.98%   +0.09%     
==========================================
  Files          24       23       -1     
  Lines        3709     3654      -55     
==========================================
- Hits         2926     2886      -40     
+ Misses        582      569      -13     
+ Partials      201      199       -2
Impacted Files Coverage Δ
source_manager.go 91.41% <100%> (+0.03%)
analysis.go 85.29% <ø> (-0.01%)
cmd.go
trace.go 73.19% <ø> (+0.97%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f8d2e5...517d749. Read the comment docs.

@sdboyer
Copy link
Owner

sdboyer commented Jan 27, 2017

I appreciate the PR, but this is intentional. Assuming the calling code has created the *SourceMgr through NewSourceManager() and is not messing with unsafe, it's not possible to reach that point in the logic and have that file handle be nil.

Or at least, it shouldn't be possible. If it does happen, it indicates the code has followed a significant different path than I thought possible, some correctness concerns may have been violated, and there is no knowable way to recover, and no way of communicating this back to the calling code, so a panic is merited. Silently letting it go would not be great.

However, being that this is intentional, I'd accept a version of this PR where we explicitly panic with an informative message if it is nil.

@sdboyer
Copy link
Owner

sdboyer commented Jan 27, 2017

If you're willing to do that, please also add a test case for it - that really codifies that it's intentional.

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

Successfully merging this pull request may close these issues.

None yet

3 participants