On Mon, 24 Feb 2025 13:08:49 GMT, David Beaumont <d...@openjdk.org> wrote:
>> 8349206: j.u.l.Handler classes create deadlock risk via synchronized
>> publish() method.
>>
>> 1. Remove synchronization of calls to publish() in Handlers in
>> java.util.logging package.
>> 2. Add explanatory comments to various affected methods.
>> 3. Add a test to ensure deadlocks no longer occur.
>>
>> Note that this change does not address issue in MemoryHandler (see
>> JDK-8349208).
>
> David Beaumont has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Tweaking @implNote for better rendering.
src/java.logging/share/classes/java/util/logging/Formatter.java line 94:
> 92: * important to avoid making callbacks to unknown user-provided
> arguments
> 93: * (e.g. log record parameters captured from previous calls to
> 94: * {@link #format(LogRecord)}.
I'm having trouble getting my head around these comments. Are they intended for
callers or for subclassers? Is the "associated" handler the same handler as the
argument, or a different one? I saw the discussion of potential (largely
theoretical?) deadlock in the discussion in StreamHandler.publish(), but I'm
having trouble figuring out what kind of actual constraint that potential
imposes on which code.
src/java.logging/share/classes/java/util/logging/Handler.java line 62:
> 60: * {@link #isLoggable(LogRecord)} while allowing code synchronized on the
> 61: * {@code Handler} instance to set a new log level.
> 62: * <p>
The above discussion is mostly internal to Handler implementations so I don't
think it belongs in public javadoc. The implementation comments at line 91
below cover this (though they could be clarified a bit) but I think the
mutables-volatile/unlocked-get/locked-set convention is mostly orthogonal to
the deadlock issue we're trying to solve here. I'd recommend simply removing
this material.
As an aside I think using volatiles this way is quite error-prone and I don't
like advertising it in the javadoc.
src/java.logging/share/classes/java/util/logging/Handler.java line 80:
> 78: * {@link FileHandler} or {@link Formatter} then, in order to have
> complete
> 79: * control of synchronization, it is recommended to subclass {@code
> Handler}
> 80: * directly.
The discussion here belongs on StreamHandler.publish(), because its
implementation does lock on the StreamHandler instance itself. As such there
isn't any other discussion of locking in the specs for this class, so it seems
kind of misplaced.
src/java.logging/share/classes/java/util/logging/Handler.java line 163:
> 161: * <p>
> 162: * @implNote Implementations of this method should avoid holding any
> locks
> 163: * around the formatting of {@code LogRecord}.
I actually do not think this is correct. It's safe to hold locks as long as no
other path into this class attempts to take the same lock, which gives rise to
the lock ordering problem.
src/java.logging/share/classes/java/util/logging/StreamHandler.java line 184:
> 182: *
> 183: * @param record description of the log event. A null record is
> 184: * silently ignored and is not published
There needs to be an `@implSpec` somewhere in this method's javadoc comment
that explains the locking policy here very crisply for subclassers.
Specifically (1) the lock on `this` is not held during formatting; (2) the lock
on `this` is held during publishing; (3) subclassers must not lock on `this`
while calling super.publish() because it would contravene (1).
src/java.logging/share/classes/java/util/logging/StreamHandler.java line 268:
> 266: }
> 267:
> 268: // Called synchronously.
I would be more specific here and say that it's called while the lock on `this`
is held.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1968668798
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1968657704
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1968660845
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1968662121
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1968664835
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1968662606
