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

[David Beaumont]

On Tue, 25 Feb 2025 01:27:04 GMT, Stuart Marks <smarks at 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