On Tue, 25 Feb 2025 01:18:35 GMT, Stuart Marks <[email protected]> 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
