Sorry, I meant not GridCacheLockImpl but CacheLockImpl. пт, 3 апр. 2020 г. в 17:04, Ivan Daschinskiy <ivanda...@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
