Skip to content
This repository has been 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
merged 1 commit into from
Mar 10, 2017

Conversation

spenczar
Copy link
Contributor

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

codecov bot commented Mar 10, 2017

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
Owner

sdboyer commented Mar 10, 2017

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

@spenczar
Copy link
Contributor Author

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
Contributor Author

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
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 😄

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.

2 participants