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

[Jason Mehrens]

On Mon, 17 Feb 2025 13:52:33 GMT, David Beaumont <duke at openjdk.org> wrote:

>>>I've looked through a lot of github logging properties files and see no evidence that the pattern of use...
>> 
>> Closed source GOTS/COTS products that require troubleshooting require using all of the logging API features when other solutions are forbidden for non-tech reasons.
>> 
>> To the point though, limit one with a count makes FileHandler work like the MemoryHandler in that at the end of say a batch run you have exactly the last N records in the form of N files.  That is a feature.
>> 
>>>The reason I'm pushing back a little here is that the idea of making a "secret handshake"
>> 
>> The behavior is what i care about not the implementation.  I would rather focus on fitting your change and retain old behavior. Behaviorly, i think deadlock can be fixed with retaining existing log rollover behavior. Secret handshake off the table then. Done and dead.
>> 
>> New idea. All JDK subclass of StreamHandler call flush.
>> 
>> 1. Put rotate check in FileHandler::flush
>> 2. Make StreamHandler call flush while holding lock.
>> 3. Remove explicit flush call in subclasses.
>> 4. Add auto flush property to StreamHandler, default false.
>> 5. Set auto flush true in subclass.
>> 
>> 
>>>..."EncryptedLogFileHandler" that's not a sub-class of FileHandler.
>> 
>> That example does apply because if it is not a FileHandler there is no check to call rotate method.  What am I missing?
>
> The issue with a non-JDK stream handler subclass, is that it doesn't have the privilege of being able to do anything before the `super.publish(...)` call has finished and the handler instance is unlocked.
> 
> Using a package protected method to achieve this only for JDK classes is just leaving a slightly bad taste in my mouth.
> 
> There's just nothing in the API, other than perhaps brittle things like using an overridden stream and detecting writes (which Daniel has convinced me is too brittle to be in the JDK) to allow a non-JDK version of FileHandler to be implemented with "synchronous post publish() actions". And I personally dislike the idea that people can't write their own equivalent of FileHandler (with some extra feature) outside the JDK.
> 
> And, just FYI, there's basically almost no chance we'd consider adding new methods or properties to the logging package at this point. I'd be far more scared about adding "auto flushing" to existing handler sub-classes than changing the never specified behaviour with respect to file size (though the pathological 1-byte file to force rotation per log record is something I hadn't considered, but has also never been promised to actually work).

My notes from 2006 on post publish actions suggested promoting `public void push` to StreamHandler and perhaps Handler as a handler defined action. Same for pushLevel but that is not radically different from auto flush I just proposed.

Another option here is to check written before and after super.publish in FileHandler. That way when 2nd record knows a rotate is required before writing. The if check is a bit different between before and after.  That should cover all scheduling combinations.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1958964838