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

[Stuart Marks]

On Fri, 14 Feb 2025 15:07:58 GMT, David Beaumont <duke at openjdk.org> wrote:

>> Well spotted :) Yes, it's a theoretical risk, but not at the same level as that of log record formatting.
>> 
>> My original draft push of this PR had comments about lock expectations here, but I was asked to not change the JavaDoc on Formatter.
>> 
>> The getHead() and getTail() methods *could* cause deadlock, but only because of code directly associated with their implementation. They don't have any access to a log record (and no reason to have access to log records), so they aren't going to be calling into completely arbitrary user code.
>> 
>> It's also unlikely that a formatter implementation will do a lot of complex work in these methods since, semantically, they are called an arbitrary number of times (according to configuration) and at arbitrary times, so they really cannot meaningfully rely on user runtime data or much beyond things like timestamps and counters.
>> 
>> So while, in theory, it could be an issue, it's an issue that can almost certainly be fixed by the author of the Formatter class itself. This is contrasted with the issue at hand, which is inherent in the handler code and cannot be fixed in any other reasonable way by the user of the logging library.
>> 
>> I'd be happy to move the head/tail acquisition out of the locked regions if it were deemed a risk, but that's never something I've observed as an issue (I spent 10 years doing Java logging stuff and saw the publish() deadlock, but never issues with head/tail stuff). It's also hard to move it out, because tail writing happens in close(), called from flush(), both of which are much more expected to be synchronized, so you'd probably want to get and cache the tail() string when the file was opened.
>
> As for the example you gave there, that is interesting. Keeping an unformatted log record around for any time after the log statement that created it has exited would be quite problematic (it prevents GC of arbitrary things).
> 
> If I were doing some kind of summary tail entry, I'd pull, format (user args) and cache anything I needed out of the log record when I had it, but not keep it around. Then when the tail is written I'd just have what I need ready to go.
> 
> But yes, right now, with or without this PR, that code looks like it's got a deadlock risk.

Need to update to have single call to getFormatter().

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

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