On Fri, 14 Feb 2025 06:14:34 GMT, Jason Mehrens <[email protected]> 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
