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

[David Beaumont]

On Tue, 25 Feb 2025 01:18:35 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>> 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/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.

Well that's true for any locking, but `publish()` is essentially required (unless you're just throwing away information) to call `toString()` or similar on the arbitrary parameters it's passed.
You can't squirrel away the parameters and format them later (that's a serious issue for mutable parameters) so you must process them before this method exits (yes, you could do it in another thread and block until processing is done, but you can't exit the log statement until it is).

So, in all but the most specialized implementations of `publish()` you are expected to be calling `toString()` or arbitrary user provided objects. And if you do that, you are absolutely at risk of deadlock (see the issue description for an example).

I mean you could explain how there are a tiny number of special case situations in which technically you'll get away with holding a lock while formatting a log record, but this does say "should" and not "must", so I think it's worded fairly.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1969572397