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

[rahul2393]

Fixes #5710.

[yfuruyama]

trackedSessionHandles could be being modified in different places like here. https://github.com/googleapis/google-cloud-go/blob/spanner/v1.30.0/spanner/session.go#L83

How do we guarantee that we will dump a consistent snapshot of trackedSessionHandles without locking p.mu?

[rahul2393]

I think getTrackedSessionHandleStacksLocked is only called when the client gets errGetSessionTimeout, so even if trackedSessionHandles later gets updated as a customer I want to know what happen when errGetSessionTimeout was thrown and not the current snapshot, so I think we should release the lock as soon as we read trackedSessionHandles.Front()

[rahul2393]

@codyoss can you please help review this.

[yfuruyama]

@rahul2393 If I understand correctly, this could print invalid state as stack traces.

Let's say there are the following session handle chains in trackedSessionHandles when the client gets errGetSessionTimeout. And same time, let's say shA was recycled by other goroutine.

shA -> shB -> shC

Then valid state would be either shA -> shB -> shC or shB -> shC depending on timinig.

But if we allow other goroutine to get shA removed from trackedSessionHandles, then the returned value from p.trackedSessionHandles.Front() would loose next pointer as Go's list.List.Remove removes the next pointer from the element. So the stack trace would be just shA for this case.

[rahul2393]

you are right, I updated the PR @yfuruyama can you check if its ok

[yfuruyama]

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

[rahul2393]

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

[rahul2393]

Do you find any issue with it?

[yfuruyama]

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

[olavloite]

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()

[ansh0l]

lgtm

[olavloite]

The solution itself LGTM, but take a look at my suggestion to reorganize the function a little to avoid releasing and taking the lock multiple times.