On Wed, 12 Feb 2025 19:41:44 GMT, Jason Mehrens <d...@openjdk.org> wrote:
>> 8349206: j.u.l.Handler classes create deadlock risk via synchronized
>> publish() method.
>>
>> 1. Remove synchronization of calls to publish() in Handlers in
>> java.util.logging package.
>> 2. Add explanatory comments to various affected methods.
>> 3. Add a test to ensure deadlocks no longer occur.
>>
>> Note that this change does not address issue in MemoryHandler (see
>> JDK-8349208).
>
> 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 sum 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().
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23491#discussion_r1953326840
