LockManager AST test

[asfernandes]

No description provided.

[asfernandes]

Is this the correct pattern to use the lock manager with ASTs?
Without both globalMutex and localMutex, problems appears.
I can't see why globalMutex should be necessary.
On the other hand, localMutex seems necessary by design, as enqueue method locks something but only a moment later the lock id is saved in the lock object, which is the object registered as parameter of the AST routine.

[hvlad]

Didn't run it yet, so just a few notes after reading the code:

• so far I also see no need in globalMutex
• localMutex is necessary as it protect threadData internals
• localMutex should be unlocked/locked in Callbacks::checkoutRun()

[asfernandes]

I added the localMutex checkout, but globalMutex is still required to not crash.

[hvlad]

I found two problems.

First one is the worker thread shutdown sequence:

• locks are deleted before shutdownOwner() and leave possibility for AST's to access already freed object,
• shutdownOwner() is called with callbacks and unlocked mutex and, most important, it waits for AST's delivery inside lock manager (here callback's mutex is used). This allows AST's to dequeue already dequeued lock request.

Correct sequence is to dequeue locks, call shutdownOwner() with mutex locked, and then delete locks finally.
Also, AST handler should know that lock is already dequeued and don't call dequeue() second time.

Second issue is related with enqueue():

• there is possibility that AST handler will run after lock request is granted but before its id is known (assigned) to the external lock object. In our case AST handler call dequeue() with lockId == 0.

Attached patch fixed all this issues: lm_ast_test.patch

PS I've added threadId and ownerHandle to the ThreadData to make troubleshooting more easily.

[hvlad]

Fixed patch, first one missed lock->locked = true;
lm_ast_test.patch

[asfernandes]

Thanks!
Now some doubts related to LCK interfaces.
LCK_lock internal and its usage seems to not have such treatment of check lck_id in AST or check blocking after lock is acquired.
So, how AST are not missed when engine code uses LCK_lock?

[hvlad]

LCK is (almost) simple wrapper around lock manager API.
Similar checks and flags are implemented within the objects that own the locks. Many objects uses flags like blocking.
Race condition when lock is acquired first time and released by AST before lock_id is assigned is not common and was fixed in the past. It was with monitoring lock, iirc

[hvlad]

Second include of the same file