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.

Sign up for GitHub

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

Jump to bottom

Wire ctx to getdag operations in gc.GC #2221

Merged
merged 1 commit into from
Feb 5, 2016
Merged

Wire ctx to getdag operations in gc.GC #2221

merged 1 commit into from
Feb 5, 2016

Conversation

rht
Copy link
Contributor

@rht rht commented Jan 21, 2016

@rht rht added the RFCR label Jan 21, 2016
@jbenet
Copy link
Member

jbenet commented Jan 24, 2016

cc @whyrusleeping pls CR

@jbenet
Copy link
Member

jbenet commented Jan 24, 2016 

ok    github.com/ipfs/go-ipfs/util/ipfsaddr 0.028s
?     github.com/ipfs/go-ipfs/util/peerset  [no test files]
?     github.com/ipfs/go-ipfs/util/pipes  [no test files]
?     github.com/ipfs/go-ipfs/util/sadhack  [no test files]
?     github.com/ipfs/go-ipfs/util/testutil [no test files]
?     github.com/ipfs/go-ipfs/util/testutil/ci  [no test files]
ok    github.com/ipfs/go-ipfs/util/testutil/ci/travis 0.016s
?     github.com/ipfs/go-ipfs/util/todocounter  [no test files]
panic: close of closed channel

goroutine 31 [running]:
github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.(*hangwriter).Close(0xc820098020, 0x0, 0x0)
  /Users/travis/gopath/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/writer_test.go:28 +0x30
github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.(*bufWriter).die(0xc820088780)
  /Users/travis/gopath/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/writer.go:185 +0x58
github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.(*bufWriter).loop(0xc820088780)
  /Users/travis/gopath/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/writer.go:232 +0x2e2
created by github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.newBufWriter
  /Users/travis/gopath/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/writer.go:151 +0xbf

goroutine 1 [chan receive]:
testing.RunTests(0x278548, 0x332560, 0x7, 0x7, 0xc82004de01)
  /usr/local/go/src/testing/testing.go:562 +0x8ad
testing.(*M).Run(0xc82004def8, 0x8b2a2)
  /usr/local/go/src/testing/testing.go:494 +0x70
main.main()
  github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/_test/_testmain.go:66 +0x116

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
  /usr/local/go/src/runtime/asm_amd64.s:1721 +0x1

goroutine 18 [select]:
github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.(*MirrorWriter).logRoutine(0xc82008c340)
  /Users/travis/gopath/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/writer.go:71 +0x33c
created by github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.NewMirrorWriter
  /Users/travis/gopath/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/writer.go:38 +0xe2

goroutine 19 [chan receive]:
testing.(*T).Parallel(0xc820084360)
  /usr/local/go/src/testing/testing.go:422 +0x7c
github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log.TestContextContainsMetadata(0xc820084360)
  /Users/travis/gopath/src/github.com/ipfs/go-ipfs/vendor/QmQg1J6vikuXF9oDvm4wpdeAUvvkVEKW1EYDw9HhTMnP2b/go-log/context_test.go:10 +0x45
testing.tRunner(0xc820084360, 0x332560)
  /usr/local/go/src/testing/testing.go:456 +0x98
created by testing.RunTests
  /usr/local/go/src/testing/testing.go:561 +0x86d

@jbenet
Copy link
Member

jbenet commented Jan 24, 2016

@rht i think this LGTM. but tests first.

whyrusleeping
whyrusleeping reviewed Jan 24, 2016
View reviewed changes
repo/fsrepo/fsrepo.go
@@ -562,7 +562,9 @@ func (r *FSRepo) GetStorageUsage() (uint64, error) {

var du uint64
err = filepath.Walk(pth, func(p string, f os.FileInfo, err error) error {
du += uint64(f.Size())
if f != nil {
Copy link
Member

Choose a reason for hiding this comment

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

we really should check the error here and at least log it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is err, it will get returned by the function GetStorageUsage anyway.

Copy link
Member

Choose a reason for hiding this comment

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

but the error inside this function is different than the error returned from filepath.Walk. The error passed into the function here is one that comes from calling stat on the path we're also passed. The one from filepath.Walk is whatever we return from our function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stat is called from the filepath.Walk (https://golang.org/src/path/filepath/path.go#L368). The specific walkFn call here doesn't return any err value unless if a nil stat is considered err.

Copy link
Member

Choose a reason for hiding this comment

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

what i'm saying is that the err in our walkfunc is any error that gets returned from the lstat you link to. I'm saying that we should log that error just in case. I'm always against blanket ignoring errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The err is returned by GetStorageUsage, and is already logged by the caller.

Copy link
Member

Choose a reason for hiding this comment

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

err = filepath.Walk(pth, func(p string, f os.FileInfo, err error) error {
        if err != nil {
            log.Debugf("filepath.Walk error: %s", err)
            return nil
        }
        du += uint64(f.Size())
        return nil
    })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already https://github.com/ipfs/go-ipfs/blob/eb9d0729280669b1e277b082cc28c45c87038020/core/corerepo/gc.go#L147. I added the log, however.

@whyrusleeping
Copy link
Member

@jbenet that panic is just to do with a quick hack i'm using in testing. Nothing that will actually affect the codebase. We could do one of those fancy schmancy 'close the channel only if its not already closed' checks to make that panic never happen again though

@rht
Copy link
Contributor Author

rht commented Jan 25, 2016

We could do one of those fancy schmancy 'close the channel only if its not already closed' checks to make that panic never happen again though

Which I assume that 'close the channel only if its not already closed' is done in the rest of the codebase that is not test?

@whyrusleeping
Copy link
Member

correct. The channel closing in question is inside of a testwriter struct thats only used in testing.

@rht rht mentioned this pull request Feb 2, 2016
Closed
whyrusleeping added a commit that referenced this pull request Feb 5, 2016
@whyrusleeping
Merge pull request #2221 from rht/fix/gc-panic
0de7104 
Wire ctx to getdag operations in gc.GC
@whyrusleeping whyrusleeping merged commit 0de7104 into ipfs:master Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
3 participants
@rht @jbenet @whyrusleeping