RFR: 8349206: j.u.l.Handler classes create deadlock risk via synchronized publish() method [v6]

[Stuart Marks]

On Mon, 24 Feb 2025 13:08:49 GMT, David Beaumont <duke at 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