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

Fix err shadowing bug that caused panics for unreachable repos#187

Merged
sdboyer merged 1 commit intosdboyer:masterfrom
spenczar:fix_err_shadowing_bug
Mar 10, 2017
Merged

Fix err shadowing bug that caused panics for unreachable repos#187
sdboyer merged 1 commit intosdboyer:masterfrom
spenczar:fix_err_shadowing_bug

Conversation

@spenczar
Copy link
Copy Markdown
Contributor

@spenczar spenczar commented Mar 10, 2017

This was a little tricky: my company has a private git server hidden behind a VPN. I ran dep status while not on the VPN, and dep panicked and crashed when it tried to look up the version of one of those private repos. This seemed weird!

It turns out this comes down to an error shadowing bug, deep in the *SourceMgr's futures. The test here panics on master, but passes with this revision.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 10, 2017 •
edited
Loading

Codecov Report

Merging #187 into master will increase coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   78.96%   79.15%   +0.18%     
==========================================
  Files          24       24              
  Lines        3804     3804              
==========================================
+ Hits         3004     3011       +7     
+ Misses        600      596       -4     
+ Partials      200      197       -3
Impacted Files Coverage Δ
deduce.go 79.76% <100%> (+1.66%) ✅
source_manager.go 91.38% <0%> (ø) ✅

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 4b29d30...2c74208. Read the comment docs.

@sdboyer
Copy link
Copy Markdown
Owner

sdboyer commented Mar 10, 2017

Was the panic the same as what was seen in golang/dep#250 ?

@spenczar
Copy link
Copy Markdown
Contributor Author

spenczar commented Mar 10, 2017

It looks a little different, I'm afraid - I get it in (*SourceMgr).ListVersions, while that one is in (*SourceMgr).SyncSourceFor:

anic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x40 pc=0x133f044]

goroutine 1 [running]:
github.com/golang/dep/vendor/github.com/sdboyer/gps.(*SourceMgr).ListVersions(0xc42008cb40, 0xc42013ffa0, 0x19, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/Users/snelson/go/src/github.com/golang/dep/vendor/github.com/sdboyer/gps/source_manager.go:395 +0x154
main.runStatusAll(0x160cc00, 0xc4201aa000, 0xc420142e10, 0xc42008cb40, 0x2, 0x20)
	/Users/snelson/go/src/github.com/golang/dep/cmd/dep/status.go:284 +0x7bb
main.(*statusCommand).Run(0xc42013f6e0, 0xc420157790, 0xc42000c2e0, 0x0, 0x0, 0x0, 0x0)
	/Users/snelson/go/src/github.com/golang/dep/cmd/dep/status.go:179 +0x15c
main.main()
	/Users/snelson/go/src/github.com/golang/dep/cmd/dep/main.go:96 +0x64f

@spenczar
Copy link
Copy Markdown
Contributor Author

spenczar commented Mar 10, 2017

Looking a little closer, actually, it looks like it might have the same root cause? (*SourceMgr).getSourceFor was the bad seed that could return (nil, nil), which is what triggered the panic in golang/dep#250.

@sdboyer
Copy link
Copy Markdown
Owner

sdboyer commented Mar 10, 2017

It's likely still the same problem; the problem is a nil, nil return from sm.getSourceFor(), which is what was resulting from that shadowed err.

Again, great catch 😄

@sdboyer sdboyer merged commit 9a583ef into sdboyer:master Mar 10, 2017
This was referenced Mar 10, 2017
Closed
Closed
Closed
krisnova pushed a commit to krisnova/dep that referenced this pull request Apr 21, 2017
@sdboyer
Merge pull request sdboyer/gps#187 from spenczar/fix_err_shadowing_bug
178ffce 
Fix err shadowing bug that caused panics for unreachable repos
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@spenczar @sdboyer