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

[David Beaumont]

On Fri, 14 Feb 2025 19:51:19 GMT, Jason Mehrens <duke at openjdk.org> wrote:

>> The reason I'm pushing back a little here is that the idea of making a "secret handshake" method between StreamHandler and FileHandler isn't a solution for anyone who made their own handler (e.g. an "EncryptedLogFileHandler" that's not a sub-class of FileHandler). The shape of the existing public API makes the additional promise that "post-processing" occurs synchronously with the last log written hard/impossible, and having it so only JDK classes can do this feels a bit wrong to me.
>> 
>> In other words, if you can convince me it's worth doing something to make FileHandler "more synchronized" in this way, I think we should change the API or find another way to allow any third-party sub-classes to also solve the issue. This is why I'm seeking real world examples of actual code that appear to rely on the invariant we're discussing.
>
>>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 scarred 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).

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

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