On Tue, 25 Feb 2025 01:27:04 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Tweaking @implNote for better rendering.
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1969589330
