On Thu, 11 May 2023 08:51:45 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> 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. It's the same usage than in `PrintStream`: the lock in `PrintStream` is an `InternalLock` even though it's never exposed to subclasses (it's a private field). My rationale was that if the underlying `PrintStream` uses `synchronized` and doesn't use `InternalLock`, because `-Djdk.io.useMonitors=true`, then there's no point in the `Handler` trying to avoid using `synchronized`. Though I admit that not all `Handlers` wrap a `PrintStream`, the `FileHandler` and `ConsoleHandler` (which are the more important ready-to-use concrete implementations) will eventually delegate to some underlying IO class that will be impacted by `-Djdk.io.useMonitors=true`. So I was thinking that we could/should use the same logic there. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13832#discussion_r1190950653
