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? |
Sorry, I meant not GridCacheLockImpl but CacheLockImpl.
пт, 3 апр. 2020 г. в 17:04, Ivan Daschinskiy <[hidden email]>: > 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 |
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" < [hidden email] > >To: [hidden email] >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 < [hidden email] >: > >> 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? >> >> |
Free forum by Nabble | Edit this page |