On Tue, 25 Feb 2025 11:35:18 GMT, David Beaumont <d...@openjdk.org> wrote:
>> src/java.logging/share/classes/java/util/logging/Formatter.java line 94:
>>
>>> 92: * important to avoid making callbacks to unknown user-provided
>>> arguments
>>> 93: * (e.g. log record parameters captured from previous calls to
>>> 94: * {@link #format(LogRecord)}.
>>
>> I'm having trouble getting my head around these comments. Are they intended
>> for callers or for subclassers? Is the "associated" handler the same handler
>> as the argument, or a different one? I saw the discussion of potential
>> (largely theoretical?) deadlock in the discussion in
>> StreamHandler.publish(), but I'm having trouble figuring out what kind of
>> actual constraint that potential imposes on which code.
>
> The caller of this class is almost always JDK logging code, so these comments
> are intended for anyone implementing a formatter. The handler is the target
> handler (well spotted), and I don't believe it's ever actually null in any
> real code in the JDK.
>
> The reason that I commented here is because in the PR conversation, it's been
> noted that formatter is odd because two of its methods are intended to be
> called without locks held (and for implementations to avoid taking more
> locks), and two methods are expected to be called with locks held, which
> means any implementation of them should avoid calling toString() etc.
>
> Now while getHead() and getTail() aren't given any user arguments to format,
> it was pointed out that they might have held onto some (e.g. from the last
> processed log record). And this use case isn't entirely odd, because having a
> "getTail()" method that emits the last log statement timestamp, or some
> summary of the number of log statements of certain types has value. This can
> be done safely if you only capture timestamps and counters, because
> formatting them won't result in arbitrary user code being invoked, but it's
> easy to imagine people thinking it's also okay to capture certain logged
> arguments to processing here (while it would be safe to call toString(), and
> capture that when the record is processed, it's not safe to call toString()
> in getHead() or getTail()).
>
> I felt that since, without guidance, there's a good chance people will assume
> all the methods in this class are "the same" in terms of locking constraints,
> something needed to be put in to distinguish the two (incompatible)
> constraints these methods have.
After discussion, we decided this was too special-case and too tricky to
explain in a simple JavaDoc note, so I revert these comments for now. We might
add them back, or (more likely) I'll write a "how to write handlers and
formatters" guide in some format tbd.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1975361890
