On Fri, 5 May 2023 13:43:43 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
> Several Handlers class use monitors to synchronize when formatting / > publishing LogRecords. > When logging within a VirtualThread, holding this monitor can cause the > carrier thread to be pinned. > Handlers could use jdk.internal.misc.InternalLock, in a similar way to some > java.io classes (such as PrintStream) to avoid pinning the carrier thread. 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). Another use of synchronization is to ensure consistency. Publish uses synchronization for instance, to make sure that there is no concurrent publication of records, which could cause an inconsistent state in the output. In some cases, changing some of the fields during publication is also not desirable - and that is why setEncoding/setOutputStream have been changed to also use the same lock than publish() is using. Your question pushed me to double check that I got the logic right - which made me noticed I missed some updates in SocketHandler. So it's doubly appreciated :-) I could - for consistency - change the logic to alter every use of synchronized (including any getter/setter) to use the internal lock if present - but it didn't seem necessary, and I choose to minimize my changes. If you or other reviewers think that it would be better to switch all uses of synchronized to consider using the internal lock if present, I can do that. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13832#issuecomment-1536498908
