michael-o commented on pull request #77: URL: https://github.com/apache/maven-resolver/pull/77#issuecomment-818679028
> > > > There is one more thing I do not understand and think that this undermines reentrancy: While I understand that you maintain a concurrent map in `NamedLockFactorySupport` to manage locks (counts) between threads in one JVM to properly obtain and release them, but I fail to understand why both `AdaptedReadWriteLockNamedLock` and `AdaptedSemaphoreNamedLock` use thread locals to manage these locks. Looking at how lock and unlock are done, I don't see how underlying locks are used when reentrancy happens. Thus, the internal lock count is not increased, but you turn reentrancy into a `NOOP` and the end. Each call to `lockShared()`/`lockExclusively()` should be delegated to the underlying lock , no matter what. Please explain! > > So, while `NamedLock` is re-entrant, the underlying constructs are not always. For example, all the "semaphore-like" things are not re-entrant: each lock requires permissions to be taken from it (shared: ONE, exclusive: ALL). So, re-entrant capability with semaphores is implemented here by tracking "history" and avoiding doing steps not necessary (the condition is already met, then noop step is performed). On the other hand, with "lock-like" things same thing is done, as used `ReadWriteLock` does not state anything about re-entrant capability (is left to implementations), but here it could be considered as "optimization" as well, as none of these actually change anything on semantic. BUT, in both cases (semaphore and lock) when using some remote construct (like Hazelcast or Redisson constructs, or who knows what else may be used) all the lock/unlock calls performs remote calls, that require serialization, orchestration and all the things you really want to avoid, especially in c ase when the outcome would really not change anything (the condition you need, the lock level you want is already in your pocket). > > PS: intentionally write "semaphore-like" and "lock-like" things, as underlying constructs usually do not share common interface or ancestor, so we "adapt" them, but the term is more to express their "nature", or simpler, what is they principle how they work. I understand this now, but am not happy with for the following reasons: We have a hard requirement on reentrancy, it is up to the lock to implement that properly. If some lock does not support it, you can employ your history for that (granted), but why impose it on locks which support it out of the box? Java's locks do perfectly fine, so does Redisson. I ran Redisson builds hundreds of times and the overhead was very little. I don't see a reason to impose another layer of emulation if the underlying lock just can be used passthrough. In fact, this can even cause bugs/weird behavior if we don't use the locks as intended. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
