The "lockedObject" might be better named "lockableObject." That way it makes no assumptions about its current state. Building on this, the class might be called Lockable.
However, I also like the idea of calling the class Locker, as in a gym locker. The idea is that it holds something important. Or (since Locker also sounds like a verb form) maybe it could be called a LockBox. That implies that it wraps something, which is what we want. I may come up with better suggestions if I could see the class. It's not in master, and I don't see it on any branch. Can you provide a link? — Miguel Muñoz On Fri, Jun 26, 2020 at 7:07 AM Gary Gregory <garydgreg...@gmail.com> wrote: > > Hi All: > > I know email is a challenging medium for code reviews, so please consider > these comments coming from my best intentions, constructive and caring ;-) > Also please excuse the meandering nature of this post. > > The new class org.apache.commons.lang3.concurrent.Locks.Lock needs better > names IMO, not just the class name. There is already a JRE interface called > Lock so this class name is confusing (to me.) > > The Javadoc reads in part: > > * This class implements a wrapper for a locked (hidden) object, and > * provides the means to access it. The basic idea, is that the user > * code forsakes all references to the locked object, using only the > * wrapper object, and the accessor methods > > But this is not the case, the object itself is not locked in > the traditional sense with a monitor through a synchronized block, so we > need to update the Javadoc as well. > > To me, this is more about executing blocks of code (through lambdas) which > then get 'safe' (via locking) access to an underlying object. This tells me > the class is more about the functional interfaces than about a domain > "Object", hence the name "Lock" is not the best. It's really more of a > visitor where the visitation pattern is: Lock, Visit, Unlock. > Instead, maybe: > - StampledLockVisitor (more specific) > - LockingVisitor (more general) > - SafeObject (vague) > - ObjectLocker (vague) > - rejected: LockedObject since the object itself is not locked. > - ? > > What is also confusing IMO is that the instance variable for the object is > called "lockedObject" but the object is in fact NOT locked all the time. So > that needs to be renamed IMO: > - object (the simplest) > - subject > - domain > - target > - ? > > In the same vein, the StampedLock is named "lock" which is also confusing > since StampedLock does not implement Lock. > > Why can't the domain object be null, it's never used by the framework? > Why this: > if (t == lockedObject) { > throw new IllegalStateException("The returned object > is, in fact, the hidden object."); > } > ? > This seems like an application specific constraint that has nothing to do > with the framework. > > Now that I've considered the above, the API Locks.lock(O) is really > misnamed, because it does not lock anything, it's a factory method. > > Stepping back even more, since there is only a static inner class in Locks, > and no-hint that alternative implementations for different kind of locks > are possible, I would say we do not need Locks, all we need is what is now > called Lock. > > It's not clear why some methods are protected, I would make those private. > It's not like I can use or plugin a Lock, ReentrantReadWriteLock, a > ReentrantLock, or any ReadWriteLock instead of a StampedLock. I would then > make the current Lock class a standalone class which can then be used later > from a factory class (now Locks) where we can then fill in with different > implementations. > > The Javadoc talks about a "hidden" object but it is not hidden since it is > passed to all visitors! > > This test assumption is wrong: > > If our threads are running concurrently, then we expect to be > faster > than running one after the other. > > The VM or Java spec makes no such guarantee and the tests have no control > over VM scheduling. There are cases where this will fail when under heavy > load for example where the cost of additional threads becomes overwhelming. > > Another item I am quite doubtful about is hiding checked exceptions by > rethrowning them as unchecked. I'd consider this an anti-pattern. If I am > using one of our "Failing" interfaces it is because I am expecting it to > fail. Perhaps this could be made smoother by refactoring to pass in a > lambda for an exception handler. > > I've crystallized my thoughts into code here as WIP (Javadoc needs work): > https://github.com/apache/commons-lang/pull/559 > > Not as important as the above: > The example in the Javadoc uses logging as its domain subject, a "logging" > API (PrintStream) which is not a good example IMO. Logging frameworks > today like Log4j handle multi-threaded applications normally without having > developers meddle in it. Yes, I understand it's a simple example but I am > hoping we can come up with something more realistic or useful. > > Thank you, > Gary