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.

Sign up for GitHub

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(spanner): early unlock of session pool lock during dumping the tracked session handles to avoid deadlock #5777

Merged
merged 4 commits into from
Mar 23, 2022
Merged

fix(spanner): early unlock of session pool lock during dumping the tracked session handles to avoid deadlock #5777

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
de2fb89
fix(spanner): early unlock of session pool lock during dumping the tr…
rahul2393 Mar 22, 2022
2cac97e
fix dirty read issue
rahul2393 Mar 22, 2022
17f49ee
Merge branch 'main' into fix-session-lock
rahul2393 Mar 23, 2022
a8b04c1
incorporate requested changes
rahul2393 Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions spanner/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ func (sh *sessionHandle) recycle() {
}
p := sh.session.pool
tracked := sh.trackedSessionHandle
sh.mu.Unlock()
sh.session.recycle()
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be wrong, but is there any possibility that sessionHandle.destory() is called during sh.session.recycle() is being called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, as per my understanding destroy is only called when isSessionNotFoundError is returned from server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you find any issue with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I was just wondering if it's ok to release a lock during sh.session.recycle() call. It looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be safer and clearer if you would just store sh.session in a local variable s while the lock is still taken, and then after (original) line 80, call s.recycle(). So you get something like:

	p := sh.session.pool
	tracked := sh.trackedSessionHandle
	s := sh.session
	sh.session = nil
	sh.trackedSessionHandle = nil
	sh.checkoutTime = time.Time{}
	sh.stack = nil
	sh.mu.Unlock()
        s.recycle()

sh.mu.Lock()
sh.session = nil
sh.trackedSessionHandle = nil
sh.checkoutTime = time.Time{}
Expand Down