ELY-2982 IdentitySharedExclusiveLock.lockExclusive() and lockShared() are uninterruptable#2369
ELY-2982 IdentitySharedExclusiveLock.lockExclusive() and lockShared() are uninterruptable#2369RanabirChakraborty wants to merge 1 commit intowildfly-security:2.xfrom
Conversation
|
@pferraro please review the PR |
darranl
left a comment
darranl
left a comment
There was a problem hiding this comment.
I think we also need the review from Paul to check this addresses his concern but in the meantime the exception needs moving to ElytronMessages.
| } | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RealmUnavailableException("Interrupted while waiting for identity lock", e); |
There was a problem hiding this comment.
FYI the Exception itself should be defined in ElytronMessages
RanabirChakraborty
force-pushed
the
ELY-2982
branch
from
e967480 to
c2f1145
Compare
… are uninterruptable
RanabirChakraborty
force-pushed
the
ELY-2982
branch
from
c2f1145 to
5a98a67
Compare
| private int sharedHoldCount; | ||
| private boolean isExclusiveLocked; | ||
| private int exclusiveRequests; | ||
| private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock(true); |
There was a problem hiding this comment.
If I understand this correctly, the change proposed here will now throw an IllegalMonitorStateException if the thread invoking IdentityLock.unlock() is not the same as the thread that originally acquired the read or write lock.
I don't know enough about the usage of this class to determine whether this new constraint is acceptable. @darranl ?
There was a problem hiding this comment.
What i know,ReentrantReadWriteLock enforces thread ownership, meaning the thread that acquires the lock must be the one to release it.
In the context of FileSystemSecurityRealm, this constraint is acceptable. These locks are used to safeguard synchronous file I/O operations. The thread that loads the identity is always the same thread that closes it, typically within a single management operation. Therefore, enforcing this relationship is safe and aligns with the intended architecture. I can be wrong, @darranl let me know what to do here.
There was a problem hiding this comment.
@skyllarr I'm waiting for a reply here. Based on that I can continue.
| IdentitySharedExclusiveLock.this.release(this); | ||
| valid = false; | ||
| public void release() { | ||
| synchronized (this) { |
There was a problem hiding this comment.
A ReentrantLock can only be unlocked by the thread that locked it, so there is no reason for memory fencing here.
|
|
||
| private final boolean exclusive; | ||
| private final Lock internalLock; | ||
| private volatile boolean valid = true; |
There was a problem hiding this comment.
This does not need to be volatile - only the owning thread can call Lock.unlock() and thus the conditional update of this value is only applicable to that thread.
|
Hi, is this still being worked on? I haven’t seen any updates in a while. If not, I’d be happy to take this over. |
|
Hi @RanabirChakraborty , is this being worked on or can @Zhang-Charlie work on it now? |
5 participants
Issue: https://issues.redhat.com/browse/ELY-2982