[SERVER-39092] ReplicationStateTransitionLockGuard should be resilient to exceptions thrown before waitForLockUntil() Created: 18/Jan/19  Updated: 29/Oct/23  Resolved: 07/Mar/19

Status: Closed
Project: Core Server
Component/s: Replication
Affects Version/s: None
Fix Version/s: 4.1.9

Type: Bug Priority: Major - P3
Reporter: Suganthi Mani Assignee: Suganthi Mani
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
Duplicate
duplicates SERVER-39425 Lock acquisition timeout should alway... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Repl 2019-02-25, Repl 2019-03-11
Participants:
Linked BF Score: 58

 Description   

In ReplicationStateTransitionLockGuard destructor, we have an invariant which checks for  lock result not equal to LOCK_WAITING before unlocking the rstl lock. But, for the below valid event sequence,  we would be calling the ReplicationStateTransitionLockGuard destructor with _result set as "LOCK_WAITING" to unlock the rstl lock.

1) Thread A  issues stepdown cmd ( can be triggered either by heartbeat or user) .

2) Thread B issues conditional stepdown triggered by the user.

3) Thread A marks thread B as killed.  (EDIT : One step down thread cannot mark another step down thread as killed, because during step down we currently kill only user operations that have taken global lock in X, IX and IS mode. Step down thread only try to take RSTL lock in X mode and not global lock.)

3) Thread A acquires the rstl lock in X mode.

4) Thread B enqueues the rstl lock and set the _result as LOCK_WAITING.

5) Thread B calls ReplicationStateTransitionLockGuard::waitForLockUntil with non-zero timeout.

6) Thread B wait for rstl lock is interrupted (or even for time out case) and lead to calling ReplicationStateTransitionLockGuard destructor with _result as "LOCK_WAITING" leading to invariant failure. (EDIT: Thread B can time out waiting for the lock).

Note: There is no need to worry that the rstl lock state won't be cleaned up, because unlockOnErrorGuard in LockerImpl::lockComplete will clean up the state in the lock manger and in the locker on any failed lock attempts. Effectively when we hit the ReplicationStateTransitionLockGuard destructor, there is nothing to clean up for the above scenario.



 Comments   
Comment by Githook User [ 07/Mar/19 ]

Author:

{'name': 'Suganthi Mani', 'email': 'suganthi.mani@mongodb.com', 'username': 'smani87'}

Message: SERVER-39092 ReplicationStateTransitionLockGuard should be resilient to exceptions thrown before waitForLockUntil().
Branch: master
https://github.com/mongodb/mongo/commit/91daf30b31e60048025162663ccb4f40cc24a837

Comment by Suganthi Mani [ 01/Mar/19 ]

Spoke to siyuan.zhou, we still need to be resilient thrown between ReplicationStateTransitionLockGuard::_enqueueLock and ReplicationStateTransitionLockGuard::waitForLockUntil, so reopening the ticket.

Comment by Siyuan Zhou [ 28/Feb/19 ]

Talked with xiangyu.yao, I don't think his work fixed all the problem. SERVER-39425 only fixed the case where the exception is thrown from lockRSTLComplete(). However, it's possible that an exception is thrown before calling into waitForLockUntil(). suganthi.mani's proposal above seems fine to me.

Comment by Suganthi Mani [ 28/Feb/19 ]

This problem got fixed as a part of this ticket SERVER-39425. And, the solution used to fix the problem is to reset the ReplicationStateTransitionLockGuard:_result to LOCK_INVALID before calling lockRSTLComplete. So, there is nothing to do for this ticket.

Comment by Siyuan Zhou [ 28/Jan/19 ]

Discussed with Geert. Suganthi's suggestion sounds good to him. He will be happy to review the patch.

Comment by Siyuan Zhou [ 19/Jan/19 ]

geert.bosch, does suganthi.mani's suggestion to always call unlock() sound good to you? It would be easier if we can call unlock blindly without calling lockComplete().

Alternatively, as we discussed last time, we could call lockComplete() with 0 timeout in the destructor. This also means we need an ON_EXIT_GUARD in waitForLockUntil() to reset _result if waitForLockUntil() throws any exception. Is lockComplete() exception-free if it's called in destructor?

As Suganthi mentioned, two concurrent stepdown may trigger this scenario, so we probably need to backport the change of GlobalLock to earlier versions.

Comment by Suganthi Mani [ 18/Jan/19 ]

My solution to this problem would be removing the below checks that we currently have in ReplicationStateTransitionLockGuard destructor before unlocking the rstl lock resource.

1) The invariant check that checks result for LOCK_WAITING.

2)  And, isLocked() check.

So, ReplicationStateTransitionLockGuard::_unlock in the destructor would be just calling LockerImpl::unlock. LOCK_WAITING invariant was added to avoid any exceptions between lock enqueue and waitForLockUntil. I don't know why we want to prevent this. Even if there is any exception happened between the _enqueue() and waitForLockUntil(), the ReplicationStateTransitionLockGuard destructor will be called  and now since check no: 2 is removed, we would call LockerImpl::unlock which would internally calls LockerImpl::_unlockImpl that would clean the lock state in the locker and in the global lock manger. 

LockerImpl::unlock would avoid calling LockerImpl::_unlockImpl only in case we run ReplicationStateTransitionLockGuard in a WriteUnitOfWork. But, currently I don't think of a case where ReplicationStateTransitionLockGuard runs in a WriteUnitOfWork. If that's the case, we are safe. Otherwise, I feel, it can lead to some issue wrt to this counter _numResourcesToUnlockAtEndUnitOfWork. So, it would be good to add this invariant in the ReplicationStateTransitionLockGuard constructor.

 invariant(!inAWriteUnitOfWork());

Also, we don't want to worry about the unlock called being multiple times( like scenarios mentioned in the description where unlockOnErrorGuard  might have cleaned the lock state) as we already have a check that prevents us from unlocking multiple times

FYI, I know rstl lock guard is modeled after global lock. But, even global lock destructor does not have check 1.

siyuan.zhou /boschg@mac.com, let me know your thoughts.

Comment by Suganthi Mani [ 18/Jan/19 ]

It seems samy.lanka got a BF-11847 imitating the above scenario.

Generated at Thu Feb 08 04:51:00 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.