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

[David Beaumont]

On Fri, 14 Feb 2025 06:14:34 GMT, Jason Mehrens <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).
>
> src/java.logging/share/classes/java/util/logging/StreamHandler.java line 210:
> 
>> 208:                 if (!doneHeader) {
>> 209:                     writer.write(getFormatter().getHead(this));
>> 210:                     doneHeader = true;
> 
> Should getHead and or getTail also not be called while holding lock?
> 
> I format a single record in: https://eclipse-ee4j.github.io/angus-mail/docs/api/org.eclipse.angus.mail/org/eclipse/angus/mail/util/logging/CollectorFormatter.html#getTail(java.util.logging.Handler)

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.

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

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