RFR: 8307535: java.util.logging.Handlers should be more VirtualThread friendly

[David M. Lloyd]

On Fri, 5 May 2023 16:31:18 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

> Thanks for your observations David!
> 
> Use of synchronization in these handler classes has two purposes: one is visibility. Since Handlers are used by multiple (logging) threads, and potentially configured from different threads (e.g. main thread), it is important that changes to configuration made by one thread are visible to other threads. To achieve this - getter and setter use the synchronized keyword - and from my analysis - it didn't seem required to change these to use a Lock, especially since these methods just set or get fields, without calling out methods on other objects (that may further down the road cause the thread to park).

I believe this is not strictly accurate: the fields I referred to (specifically `filter`, `formatter`, `logLevel`, `errorManager`, and `encoding`) are all `volatile`. There's also a note:


    // We're using volatile here to avoid synchronizing getters, which
    // would prevent other threads from calling isLoggable()
    // while publish() is executing.
    // On the other hand, setters will be synchronized to exclude concurrent
    // execution with more complex methods, such as StreamHandler.publish().
    // We wouldn't want 'level' to be changed by another thread in the middle
    // of the execution of a 'publish' call.


So, `synchronized` on these methods does not affect their atomicity (they're already all simple volatile writes) nor the visibility of the values which are written (again, volatile). The only effect it has then is to serialize access to these fields with respect to publication or other accesses - something which is explicitly bypassed when using the internal lock with publication (in the case, since the publication lock and the access lock for these fields would not be the same lock. Thus, when the internal lock is used, acquiring the monitor on these methods has no purpose, which is why I made the suggestion. It is in the end just an optimization though.

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

PR Comment: https://git.openjdk.org/jdk/pull/13832#issuecomment-1536542516