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

internal/client: remove the lock for idle connection recycle #278

Merged
merged 4 commits into from
Aug 23, 2021

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Aug 20, 2021

After mock testing, I find #270 is not sufficient to solve the problem.
To my surprise, RLock can block RLock in some case

The final decision is to reduce the granularity of the lock.
Just lock the modification of the map...

There is a risk that an idle connection become active again.
Since recycleMu is removed to protect such case, in very rare cases, someone maybe using the connection and the connection is closed by the recycle operation.

I think the outcome is acceptable, comparing to the recycle operation blocks the SendRequest() function.

Signed-off-by: tiancaiamao tiancaiamao@gmail.com

Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
@tiancaiamao
Copy link
Contributor Author

Update: getConnArray will check isIdle first, this can avoid the recycle goroutine remove active connections unexpectedly.

Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>

// An idle connArray will not change to active again, this avoid the race condition
// that recycling idle connection close an active connection unexpectedly (idle -> active).
if array.batchConn != nil && array.isIdle() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirmed return error will not make TiDB unavailable.
This error will be recognized as a RPC error, and TiDB will refresh the region cache ... and retry the request.

Copy link
Member

Choose a reason for hiding this comment

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

I confirmed return error will not make TiDB unavailable.
This error will be recognized as a RPC error, and TiDB will refresh the region cache ... and retry the request.

This behavior is a magic, it's hard to figure out that tidb will retry the request while reading the source code. Could you add some notes here and file an issue? Also, we should do refactor to make sure we know when should return a "retry" error.

@lysu
Copy link
Contributor

lysu commented Aug 26, 2021

do we need cherry-pick this to 5.0, 4.0?

@tiancaiamao
Copy link
Contributor Author

do we need cherry-pick this to 5.0, 4.0?

I'm not sure, the case is not so common. @lysu

tiancaiamao added a commit to tiancaiamao/client-go that referenced this pull request Aug 31, 2021
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
daimashusheng pushed a commit to daimashusheng/client-go that referenced this pull request Sep 2, 2021
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
@tiancaiamao
Copy link
Contributor Author

TiDB issue traced in pingcap/tidb#32500

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