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

Fix raftState close panic #5016

Merged
merged 1 commit into from
Dec 8, 2015
Merged

Fix raftState close panic #5016

merged 1 commit into from
Dec 8, 2015

Conversation

oiooj
Copy link
Contributor

@oiooj oiooj commented Dec 7, 2015

if meta data directory is not writable,it will panic (I print the error to debug):

2015/12/07 20:15:19 InfluxDB starting, version 0.9, branch unknown, commit unknown, built unknown
2015/12/07 20:15:19 Go version go1.5.1, GOMAXPROCS set to 4
2015/12/07 20:15:20 Using configuration at: /tmp/influxdb.conf
[metastore] 2015/12/07 20:15:20 Using data dir: /root/test
open meta store: mkdir all: mkdir /root: permission denied
[registration] 2015/12/07 20:15:20 registration service terminating
[retention] 2015/12/07 20:15:20 retention policy enforcement terminating
[monitor] 2015/12/07 20:15:20 shutting down monitor system
[handoff] 2015/12/07 20:15:20 shutting down hh service
[subscriber] 2015/12/07 20:15:20 closed service
panic: close of nil channel

goroutine 1 [running]:
github.com/influxdb/influxdb/meta.(*localRaft).close(0xc82014dd60, 0x0, 0x0)
        /Users/Sosara/golang/src/github.com/influxdb/influxdb/meta/state.go:206 +0x35

...

and I followed the code:


        // Create the root directory if it doesn't already exist.
        if err := s.createRootDir(); err != nil {
            return fmt.Errorf("mkdir all: %s", err)
        }

        // Open the raft store.
        if err := s.openRaft(); err != nil {
            return fmt.Errorf("raft: %s", err)
        }

The raftState is not opened yet, so if you want to close the state, check the r.closing first.

After fix:

2015/12/07 20:19:07 InfluxDB starting, version 0.9, branch unknown, commit unknown, built unknown
2015/12/07 20:19:07 Go version go1.5.1, GOMAXPROCS set to 4
2015/12/07 20:19:08 Using configuration at: /tmp/influxdb.conf
[metastore] 2015/12/07 20:19:08 Using data dir: /root/test
[registration] 2015/12/07 20:19:08 registration service terminating
[retention] 2015/12/07 20:19:08 retention policy enforcement terminating
[monitor] 2015/12/07 20:19:08 shutting down monitor system
[handoff] 2015/12/07 20:19:08 shutting down hh service
[subscriber] 2015/12/07 20:19:08 closed service
run: open server: open meta store: mkdir all: mkdir /root: permission denied

no panic

@otoolep
Copy link
Contributor

otoolep commented Dec 7, 2015

@otoolep
Copy link
Contributor

otoolep commented Dec 7, 2015

CLA has been signed.

@otoolep
Copy link
Contributor

otoolep commented Dec 7, 2015

Seems reasonable to me.


if r.closing != nil {
close(r.closing)
r.wg.Wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be outside the if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@corylanou yes,This is not safe, fixed.

@otoolep
Copy link
Contributor

otoolep commented Dec 8, 2015

Good to go @corylanou ?

Thanks @oiooj

@corylanou
Copy link
Contributor

+1. Thanks!

otoolep added a commit that referenced this pull request Dec 8, 2015
Fix raftState close panic
@otoolep otoolep merged commit c6ba1bf into influxdata:master Dec 8, 2015
@otoolep
Copy link
Contributor

otoolep commented Dec 8, 2015

Thanks again @oiooj

@jwilder jwilder added this to the 0.10.0 milestone Feb 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants