Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8349920

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

XMLWordPrintable

    • Icon: CSR CSR
    • Resolution: Approved
    • Icon: P4 P4
    • 25
    • core-libs
    • None
    • behavioral
    • minimal
    • Hide
      Observable behavioral changes are expected to be limited:

      1. Minor race conditions around handlers which employ secondary log management techniques (e.g. rotating of files or flushing of buffers).

      If a third-party subclass of StreamHandler removes locking around the `publish()` method, then they will no longer be able to perform synchronous post-publish actions (e.g. rotating log files or other cleanup).

      For example, prior to the fix, a custom StreamHandler subclass could publish() a LogRecord and then synchronously determine if some post-publish action should be performed. If the overridden publish() method is no longer synchronized, the writing of the log record, while still synchronous with other writes, is decoupled from any post-publish check. This permits additional log records to be written in other threads before one of these threads decides to perform the post-publish action. The observable result in this case might be more logs written than expected before post-publish actions are performed.

      Note: Special measures have been taken to prevent FileHandler being affected by this issue, since it appears some people rely strongly on when file limit checks and rotation are performed.

      2. Possibly exposing existing user code in toString() methods to more concurrency, and thus revealing existing thread safety issues.

      By removing synchronization around the publish() method in StreamHandler, its possible that a lot of toString() code, which was previously effectively single-threaded, will now start to be run concurrently.

      If this code was previously not thread safe, and toString() was mostly only occurring due to logging, locking in the handler instance would have effectively made the toString() calls sequential.
      Thus, removing the locking around calls to toString() might now reveal previously hidden bugs.
      Show
      Observable behavioral changes are expected to be limited: 1. Minor race conditions around handlers which employ secondary log management techniques (e.g. rotating of files or flushing of buffers). If a third-party subclass of StreamHandler removes locking around the `publish()` method, then they will no longer be able to perform synchronous post-publish actions (e.g. rotating log files or other cleanup). For example, prior to the fix, a custom StreamHandler subclass could publish() a LogRecord and then synchronously determine if some post-publish action should be performed. If the overridden publish() method is no longer synchronized, the writing of the log record, while still synchronous with other writes, is decoupled from any post-publish check. This permits additional log records to be written in other threads before one of these threads decides to perform the post-publish action. The observable result in this case might be more logs written than expected before post-publish actions are performed. Note: Special measures have been taken to prevent FileHandler being affected by this issue, since it appears some people rely strongly on when file limit checks and rotation are performed. 2. Possibly exposing existing user code in toString() methods to more concurrency, and thus revealing existing thread safety issues. By removing synchronization around the publish() method in StreamHandler, its possible that a lot of toString() code, which was previously effectively single-threaded, will now start to be run concurrently. If this code was previously not thread safe, and toString() was mostly only occurring due to logging, locking in the handler instance would have effectively made the toString() calls sequential. Thus, removing the locking around calls to toString() might now reveal previously hidden bugs.
    • Java API
    • SE

      Summary

      Synchronization has been narrowed in the publish() methods of java.util.logging.Handler sub-classes to avoid deadlock risk.

      Problem

      By synchronizing around code which calls back to arbitrary user code when formatting LogRecord instances, Handler implementations will create conditions in which deadlocks can occur.

      Solution

      By narrowing the regions of code being explicitly synchronized, deadlock risk can be removed.

      This necessarily requires removal of the synchronized keywords on any implementations of the publish() in StreamHandler and its subclasses, but may also require changes within publish() method implementations to narrow any synchronized regions. As StreamHandler is publicly subclassable, this change to its publish() method must be specified and new restrictions placed on overrides of this method.

      Specification

      Documentation change to java.util.logging.Handler class-level JavaDoc:

      + <p>
+ * <h2><a id=threadSafety>Thread Safety and Deadlock Risk in Handlers</a></h2>
+ *
+ * Implementations of {@code Handler} should be thread-safe. Handlers are
+ * expected to be invoked concurrently from arbitrary threads. However,
+ * over-use of synchronization may result in unwanted thread contention,
+ * performance issues or even deadlocking.
+ * <p>
+ * In particular, subclasses should avoid acquiring locks around code which
+ * calls back to arbitrary user-supplied objects, especially during log record
+ * formatting. Holding a lock around any such callbacks creates a deadlock risk
+ * between logging code and user code.
+ * <p>
+ * As such, general purpose {@code Handler} subclasses should not synchronize
+ * their {@link #publish(LogRecord)} methods, or call {@code super.publish()}
+ * while holding locks, since these are typically expected to need to process
+ * and format user-supplied arguments.
  *
  * @since 1.4
  */

      Api note for Handler publish method:

      + * <p>
+ * @apiNote To avoid the risk of deadlock, implementations of this method
+ * should avoid holding any locks while calling out to application code,
+ * such as the formatting of {@code LogRecord}.
  *
  * @param  record  description of the log event. A null record is
  *                 silently ignored and is not published
  */
 public abstract void publish(LogRecord record);

      Specification change/clarification for the publish(LogRecord) methods in java.util.logging.StreamHandler, java.util.logging.ConsoleHandler and java.util.logging.SocketHandler:

      + * @implSpec This method is not synchronized, and subclasses must not define
+ * overridden {@code publish()} methods to be {@code synchronized} if they
+ * call {@code super.publish()} or format user arguments. See the
+ * {@linkplain Handler##threadSafety discussion in java.util.logging.Handler}
+ * for more information.

            dabeaumo David Beaumont
            dabeaumo David Beaumont
            Daniel Fuchs, Stuart Marks
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated:
              Resolved: