At first look, I’m a little surprised we’re trying to take on this
functionality. Has anyone reached out to the JDK guys to see if they’d be
interested in having it in the JDK? That said, if we approach it from that
path, we would lose the functionality in older versions of java. So maybe I
just talked myself out of the idea of putting it in the JDK....
Just wanted to stream of consciousness my initial gut vibe.
Cheers,
-Rob
> On Jun 26, 2020, at 10: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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
