You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Ivan Daschinskiy <iv...@gmail.com> on 2020/04/03 14:04:00 UTC
Possible concurrency bug in GridCacheLockImpl
Folks,
Lurking through code, it seems I found some concurrency issue in subj.
* This class contains two fields, volatile Thread lockedThread and non-volatile int cntr (used for reentrancy)
* private method incrementLockingCount() is called inside lock(). In this method we perform volatile read in assert
(assert (lockedThread == null && cntr == 0) || (lockedThread == Thread.currentThread() && cntr > 0) then increment cntr;
* In method unlock(), we firstly decrement cntr and after that if cntr equals to 0, performs volatile write to lockedThread.
Suppose execution when asserts are enabled.
T1 | T2
-------------------------------------------------------------------------------
cntr = cntr - 1 (cntr == 0) |
lockedThread = null |
| lockedThread == null && cntr == 0
| cntr = cntr + 1 (cntr == 1)
There is a happens-before edge and we have strong guarantee that reentrancy will works and cntr will definitely equals to 1;
But if assertions are disabled, something can go wrong.
Moreover, if assertions are disabled, we can allow other thread do obtain lock even if our thread holds it.
I think that this should be fixed, for example we can throw IllegalStateException, as in unlock() method.
WDYT?
Possible concurrency bug in GridCacheLockImpl
Posted by Zhenya Stanilovsky <ar...@mail.ru.INVALID>.
Ivan, i found that mentioned problem correctly highlighted, if we still not step on this rake, its probably due to some locks above, anyway it would be correct to fix this part. Are you fill the ticket ?
>
>
>------- Forwarded message -------
>From: "Ivan Daschinsky" < ivandasch@gmail.com >
>To: dev@ignite.apache.org
>Cc:
>Subject: Re: Possible concurrency bug in GridCacheLockImpl
>Date: Fri, 03 Apr 2020 17:07:22 +0300
>
>Sorry, I meant not GridCacheLockImpl but CacheLockImpl.
>
>пт, 3 апр. 2020 г. в 17:04, Ivan Daschinskiy < ivandasch@gmail.com >:
>
>> Folks,
>>
>> Lurking through code, it seems I found some concurrency issue in subj.
>>
>> * This class contains two fields, volatile Thread lockedThread and
>> non-volatile int cntr (used for reentrancy)
>>
>> * private method incrementLockingCount() is called inside lock(). In this
>> method we perform volatile read in assert
>> (assert (lockedThread == null && cntr == 0) || (lockedThread ==
>> Thread.currentThread() && cntr > 0) then increment cntr;
>> * In method unlock(), we firstly decrement cntr and after that if cntr
>> equals to 0, performs volatile write to lockedThread.
>>
>> Suppose execution when asserts are enabled.
>>
>> T1 | T2
>>
>> -------------------------------------------------------------------------------
>> cntr = cntr - 1 (cntr == 0) |
>> lockedThread = null |
>> | lockedThread ==
>> null
>> && cntr == 0
>> | cntr = cntr + 1
>> (cntr
>> == 1)
>>
>>
>> There is a happens-before edge and we have strong guarantee that
>> reentrancy will works and cntr will definitely equals to 1;
>>
>> But if assertions are disabled, something can go wrong.
>>
>> Moreover, if assertions are disabled, we can allow other thread do obtain
>> lock even if our thread holds it.
>>
>> I think that this should be fixed, for example we can throw
>> IllegalStateException, as in unlock() method.
>>
>> WDYT?
>>
>>
Re: Possible concurrency bug in GridCacheLockImpl
Posted by Ivan Daschinsky <iv...@gmail.com>.
Sorry, I meant not GridCacheLockImpl but CacheLockImpl.
пт, 3 апр. 2020 г. в 17:04, Ivan Daschinskiy <iv...@gmail.com>:
> Folks,
>
> Lurking through code, it seems I found some concurrency issue in subj.
>
> * This class contains two fields, volatile Thread lockedThread and
> non-volatile int cntr (used for reentrancy)
>
> * private method incrementLockingCount() is called inside lock(). In this
> method we perform volatile read in assert
> (assert (lockedThread == null && cntr == 0) || (lockedThread ==
> Thread.currentThread() && cntr > 0) then increment cntr;
> * In method unlock(), we firstly decrement cntr and after that if cntr
> equals to 0, performs volatile write to lockedThread.
>
> Suppose execution when asserts are enabled.
>
> T1 | T2
>
> -------------------------------------------------------------------------------
> cntr = cntr - 1 (cntr == 0) |
> lockedThread = null |
> | lockedThread == null
> && cntr == 0
> | cntr = cntr + 1 (cntr
> == 1)
>
>
> There is a happens-before edge and we have strong guarantee that
> reentrancy will works and cntr will definitely equals to 1;
>
> But if assertions are disabled, something can go wrong.
>
> Moreover, if assertions are disabled, we can allow other thread do obtain
> lock even if our thread holds it.
>
> I think that this should be fixed, for example we can throw
> IllegalStateException, as in unlock() method.
>
> WDYT?
>
>
>
--
Sincerely yours, Ivan Daschinskiy