On Thu, 11 May 2023 10:13:04 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? >> >> 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. Ah - I see that `PrintStream` lock can be accessed through SharedSecrets... Hmmm. OK - then maybe I should leave InternalLock alone and just use ReentrantLock. Let me prototype that. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13832#discussion_r1190953432
