On Thu, 11 May 2023 07:28:03 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use locks consistently
>
> src/java.logging/share/classes/java/util/logging/Handler.java line 88:
>
>> 86: return null;
>> 87: } else {
>> 88: return InternalLock.newLockOrNull();
>
> I'm surprised to see InternalLock used here. That class was created for the
> java.io area to avoid surprises when a subclass uses a RL as the lock object.
> I assume it's just convenience to use it here, that is, I don't think the
> internal lock is exposed to subclasses in the j.u.logging API.
It's the same reason here: in these classes (and before that change) the lock
is `this` which is always exposed to subclasses or external classes. If a
handler uses `InternalLock`, and an external class `synchronize(handler)` that
could cause surprising effects. My first take at this was simply using `new
ReantrantLock()` but I thought it made sense to reuse `InternalLock` instead.
After all, there would be no point in not using `synchronized` in StreamHandler
if the underlying output stream is a PrintStream for which use of InternalLock
has been disabled?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13832#discussion_r1190770759
