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

Panic on V3 close #5495

Closed
purpleidea opened this issue May 30, 2016 · 6 comments
Closed

Panic on V3 close #5495

purpleidea opened this issue May 30, 2016 · 6 comments

Comments

@purpleidea
Copy link
Contributor

I've been able to trigger a panic when I call Close(). This is with the v.3.0.0-beta.0 tag.

panic: runtime error: invalid memory address or nil pointer dereference
google.golang.org/grpc.(*ClientConn).Close(0x0, 0x0, 0x0)
    /home/james/code/gopath/src/google.golang.org/grpc/clientconn.go:288 +0x5f
github.com/coreos/etcd/clientv3.(*Client).Close(0xc820252b40, 0x0, 0x0)
    /home/james/code/mgmt/gopath/src/github.com/coreos/etcd/clientv3/client.go:98 +0xb0

The line in question 288 is the middle line here:

func (cc *ClientConn) Close() error {
    return cc.dopts.picker.Close()
}

I don't expect a panic, the only thing I think I might be doing that's extraordinary, is that I have some running watches while this is happening. IOW, I haven't run cancel() on them before I call close.

@purpleidea
Copy link
Contributor Author

And just in case it's not clear, this is all with the clientv3 API.

@gyuho
Copy link
Contributor

gyuho commented May 30, 2016

Can you write a simple example code to reproduce this?

@purpleidea
Copy link
Contributor Author

@gyuho Unfortunately, I cannot. I have a fairly large WIP branch that experiences this.

One other thing that I forgot: This is probably related to: #5491
In the panic scenario I am experiencing, I do the following:

  1. Startup server1, startup server2.
  2. Startup client1 connecting to server1, startup client2 connecting to server1.
  3. Shutdown client1 and server1.
  4. Run Close() on client2. This causes the panic.

Right after I call close, I would have created a new client and connected to server2, however I don't get that far because of the panic. Ideally I would be able to just add an endpoint like in #5491 but this is an attempt to workaround that missing feature. When server1 shuts down, it probably causes something on client2 to go nil, and then the panic happens because this case wasn't tested.

HTH!

@siddontang
Copy link
Contributor

@gyuho

I meet the same problem and find that V3 client may be not thread safe now.

func main() {
    client, err := clientv3.New(clientv3.Config{
        Endpoints:   []string{"127.0.0.1:2379"},
        DialTimeout: 3 * time.Second,
    })
    if err != nil {
        println(err)
        return
    }

    var wg sync.WaitGroup
    wg.Add(1)
    go func() {
        defer wg.Done()
        for {
            kv := clientv3.NewKV(client)
            ctx, cancel := context.WithTimeout(client.Ctx(), 3*time.Second)
            _, err := kv.Get(ctx, "/fake_key")
            cancel()
            if err != nil {
                println(err)
                return
            }
        }
    }()

    client.Close()

    wg.Wait()
}

/cc @xiang90

gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
TestKVGetErrConnClosed needs to close the client concurrently
to reproduce the issue in etcd-io#5495.
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
gyuho added a commit to gyuho/etcd that referenced this issue May 31, 2016
@gyuho
Copy link
Contributor

gyuho commented Jun 1, 2016

@purpleidea @siddontang Please try master branch. We've added a new error type rpctypes.ErrConnClosed for this case.

Thanks for reporting.

@purpleidea
Copy link
Contributor Author

Sorry for not mentioning it earlier, but I do not experience this issue any more since the patch. Thank you again!

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

No branches or pull requests

3 participants