On Fri, 14 Feb 2025 15:07:58 GMT, David Beaumont <d...@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
