The 'Locks' class is in master. My PR is in my 1st message.
Gary
On Fri, Jun 26, 2020 at 5:27 PM Miguel Muñoz <swingguy1...@gmail.com> wrote:
> 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
>
