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

flushWatcher loop may call refreshPluginState frequently when watch chan closed #49273

Closed
lcwangchao opened this issue Dec 8, 2023 · 3 comments · Fixed by #49275
Closed

flushWatcher loop may call refreshPluginState frequently when watch chan closed #49273

lcwangchao opened this issue Dec 8, 2023 · 3 comments · Fixed by #49275

Comments

@lcwangchao
Copy link
Collaborator

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

tidb/pkg/plugin/plugin.go

Lines 279 to 289 in 90e272a

func (w *flushWatcher) watchLoop() {
watchChan := w.etcd.Watch(w.ctx, w.path)
for {
select {
case <-w.ctx.Done():
return
case <-watchChan:
_ = w.refreshPluginState()
}
}
}

This above code does not handle case watchChan closed, so refreshPluginState will be called frequently when this happens.

2. What did you expect to see? (Required)

When watchChan closed , we should close watch chan

3. What did you see instead (Required)

closed chan is not handled correctly

4. What is your TiDB version? (Required)

@lcwangchao lcwangchao added type/bug This issue is a bug. sig/sql-infra SIG: SQL Infra severity/major labels Dec 8, 2023
@siddontang
Copy link
Member

@kevin-xianliu, please sort out the code that calls etcd directly, change them one by one, this is prohibited from now on.

@siddontang
Copy link
Member

@kevin-xianliu
another thing you need to know is that one of the best way to avoid misconnecting etcd is to open Authentication in etcd and only pd-client can be allowed to connect it. please add an issue to track this.

@niubell
Copy link
Contributor

niubell commented Dec 12, 2023

@kevin-xianliu another thing you need to know is that one of the best way to avoid misconnecting etcd is to open Authentication in etcd and only pd-client can be allowed to connect it. please add an issue to track this.

we don't provide uniform pd-client for Etcd now, but I think introducing the authentication mechanism for embedded Etcd is a good idea, will open an issue to track it after we sort out this module.

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

Successfully merging a pull request may close this issue.

3 participants