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

[David Beaumont]

On Tue, 18 Feb 2025 02:08:58 GMT, Jason Mehrens <duke at openjdk.org> wrote:

>> 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.

I'm afraid I don't see how the double-check idea would work if `publish()` is no longer synchronized.

Thread 1. lock(), check, no rotate, unlock() <yield>
Thread 2. lock(), check, no rotate, unlock() <yield>
Thread 1. `super.publish()` (writes past the file size limit) <yield>
Thread 2. `super.publish()` <yield>
Thread 1. lock(), check, rotate...

Essentially to have a "synchronous post publish action", the rotate() must occur in the same locked section which wrote the final log entry (the one which pushed the size over the limit). This can only occur by either:
1. Calling from StreamHandler into FileHandler explicitly (a new, package protected method but this only works for JDK classes)
2. Using some existing implicit call (e.g. making assumptions in the stream callbacks)

The other option is to just allow the lock to be dropped, and occasionally you get slightly larger files than you might expect. Since FileHandler never promised anything about file size limits, this is "in spec" and you could suggest that if someone wants stronger promises about when files are rotated etc, it's not difficult to write your own custom handler.

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

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