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 deadloop bug #102

Merged
merged 2 commits into from
Jul 19, 2022
Merged

fix deadloop bug #102

merged 2 commits into from
Jul 19, 2022

Conversation

johnlanni
Copy link
Contributor

@johnlanni johnlanni commented Jul 13, 2022

What this PR does:

Fixed an deadloop error that would occur when a zk connection was closed.

deadloop profile:

image

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


database/kv/zk/client.go Outdated Show resolved Hide resolved
@wongoo
Copy link
Contributor

wongoo commented Jul 14, 2022

@johnlanni how deadloop happens if z.Session is close?

@johnlanni
Copy link
Contributor Author

johnlanni commented Jul 14, 2022

@johnlanni how deadloop happens if z.Session is close?

case event = <-z.Session:

This channel will be closed here:
https://github.com/dubbogo/go-zookeeper/blob/f9d2183d89d5f736496b45dfaae0cb7cb4f687cc/zk/conn.go#L233

and then this select will be a deadloop since the channel will always return with ok=false

@codecov-commenter
Copy link

Codecov Report

Merging #102 (b142e34) into master (927aad2) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   74.17%   74.22%   +0.05%     
==========================================
  Files          51       51              
  Lines        4929     4932       +3     
==========================================
+ Hits         3656     3661       +5     
+ Misses       1040     1039       -1     
+ Partials      233      232       -1     
Impacted Files Coverage Δ
database/kv/zk/client.go 48.95% <100.00%> (+0.53%) ⬆️
time/timer.go 78.52% <0.00%> (+0.67%) ⬆️

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 927aad2...b142e34. Read the comment docs.

@johnlanni johnlanni requested a review from wongoo July 15, 2022 03:36
@wongoo
Copy link
Contributor

wongoo commented Jul 15, 2022

@johnlanni For the code case event = <-z.Session:, if z.Session is closed, event is nil, switch event.State seems will panic.

@johnlanni
Copy link
Contributor Author

@johnlanni For the code case event = <-z.Session:, if z.Session is closed, event is nil, switch event.State seems will panic.

event is not nil, it will be an empty struct of zk.Event, and the event.State is zero by default which means StateDisconnected

@AlexStocks
Copy link
Collaborator

@wongoo wait for your approve.

@wongoo
Copy link
Contributor

wongoo commented Jul 19, 2022

@johnlanni For the code case event = <-z.Session:, if z.Session is closed, event is nil, switch event.State seems will panic.

event is not nil, it will be an empty struct of zk.Event, and the event.State is zero by default which means StateDisconnected

@johnlanni I have approved this PR. But I wonder whether should the Session <-chan zk.Event be defined as a reference channel, like <-chan *zk.Event, to avoid memory copy. We can discuss it in another issue.

@AlexStocks
Copy link
Collaborator

@johnlanni For the code case event = <-z.Session:, if z.Session is closed, event is nil, switch event.State seems will panic.

event is not nil, it will be an empty struct of zk.Event, and the event.State is zero by default which means StateDisconnected

@johnlanni I have approved this PR. But I wonder whether should the Session <-chan zk.Event be defined as a reference channel, like <-chan *zk.Event, to avoid memory copy. We can discuss it in another issue.

@johnlanni I have merge your pr. pls open another issue and answer @wongoo 's question.

@AlexStocks AlexStocks merged commit 01d5bc0 into dubbogo:master Jul 19, 2022
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.

5 participants