On Thu, 11 May 2023 07:52:51 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> 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? > > I can revert to using plain `ReentrantLock` if you think it's preferable. > 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? The reason for InternalLock is because the Reader/Write "lock" field is exposed to subclasses and there is a possibility that a subclass could set the lock field to an instance of ReentrantLock and confusing all the locking. You don't have this issue in j.u.logging. I am not objecting to using InternalLock, just surprised to see it being used here as I had assumed you'd just create your own explicit lock when not subclassed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13832#discussion_r1190850618
