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

[David Beaumont]

On Wed, 12 Feb 2025 20:07:51 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> src/java.logging/share/classes/java/util/logging/FileHandler.java line 755:
>> 
>>> 753:         synchronized(this) {
>>> 754:             flush();
>>> 755:             if (limit > 0 && (meter.written >= limit || meter.written < 0)) {
>> 
>> I don't think we want to write to meter.written and the re-aquire the monitor to then read meter.written.  Bytes written no longer corresponds to the formatted size of the current record. It could now include the sum of other records pending a rotate.
>> 
>> Any thought on creating a package private `StreamHandler::postPublish(LogRecord)` that is called from publish while holding monitor? Then FileHandler could just override that method to invoke flush, check, and rotate.
>
> hmmm... Thanks for chiming in Jason. Good point. So what can happen here is that if multiple threads log concurrently to the same FileHandler then log records might continue to get written to the file after the limit has been reached but before the check that would rotate is performed. In pathological cases where new records continue to arrive and manage to enter the monitor before the post-publish action that rotate can do so, this could become problematic.
> 
> I think what you are proposing would work, since we control the implementation of super.publish().

We could, but I don't think it matters. This issue is the first one pointed out in the CSR, and my current feeling is that since it's stated that the limit is "best effort" and there's always the chance that the final log before rotate was massive anyway, and the risk of this sort of interleaving is low, it's not going to make things qualitatively worse than they are now.

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

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