-
Bug
-
Resolution: Unresolved
-
P4
-
25
-
None
This bug relates to a fundamental deadlock issue with the implementation and method signature of various logging Handler subclasses in the JDK (and beyond).
In particular, the publish() method is some subclasses is marked as synchronized, requiring the entire method to be locked.
The publish() method has the sole responsibility for formatting the given LogRecord and exfliltrating the formatted log message.
As such, the publish() method is expected to invoke toString() on the given user arguments.
Specifically, StreamHandler, MemoryHandler and and associated subclasses are affected.
The deadlock issue can occur for normal code which performs synchronization and logging, perhaps in separate classes.
Example:
1. Class X has methods foo() and bar(), both of which lock normally to protect normal state as an internal implementation detail.
2. Class Y has method baz() which does no locking, but will log, this is called by X.bar() while X is locked.
3. Class Z, has a toString() method which calls X.foo().
Nothing here is unreasonable, since each class is just using normal Java constructs, but now there is a deadlock risk.
In Thread 1:
1. A log statement passing an instance of Z is reached.
2. A handler subclass with a synchronized publish() method is invoked and locks the handler A.
3. toString() is invoked and calls X.foo(), which locks B.
In Thread 2:
1. Code calls X.bar() normally, which locks B and calls Y.baz().
2. Y.baz() invokes the log statement, which goes on to lock the same handler A (not unusual as handlers can be globally shared).
There is now an AB/BA deadlock risk.
None of the code involved in the above example is unreasonable or even unusual. The code doing locking (X) does no logging. The code logging (Y) does no locking, and neither knows that an instance of Z is being logged elsewhere.
Thus, the only place where the deadlock is both known about, and can be mitigated is the log handler itself, which must avoid locking around any callbacks to user code (e.g. toString(), but also via things like the Formattable interface).
Unfortunately StreamHandler uses synchronized on the entire publish() method to protect its write state, and by publishing the method as synchronized, several subclasses follow "best practice" and also synchronize it.
The fix for this bug must involve removing the synchronized keyword from all Handler implementations in the JDK and reworking them to protect their state (where necessary) without synchronizing around any code which may call back to user code.
As such, a CSR will be needed for this issue to explain the situation to users and library developers, especially since any reduction in locking around calls to toString() may cause more concurrent execution of this code and reveal non-thread-safe implementations previously hidden by virtue of being effectively single threaded during logging.
Any fix to this is easily backported if necessary.
MemoryHandler also locks its publish() method, but this class is "special" and has multiple other issues with its design, so I will raise a separate issue to cover this.
In particular, the publish() method is some subclasses is marked as synchronized, requiring the entire method to be locked.
The publish() method has the sole responsibility for formatting the given LogRecord and exfliltrating the formatted log message.
As such, the publish() method is expected to invoke toString() on the given user arguments.
Specifically, StreamHandler, MemoryHandler and and associated subclasses are affected.
The deadlock issue can occur for normal code which performs synchronization and logging, perhaps in separate classes.
Example:
1. Class X has methods foo() and bar(), both of which lock normally to protect normal state as an internal implementation detail.
2. Class Y has method baz() which does no locking, but will log, this is called by X.bar() while X is locked.
3. Class Z, has a toString() method which calls X.foo().
Nothing here is unreasonable, since each class is just using normal Java constructs, but now there is a deadlock risk.
In Thread 1:
1. A log statement passing an instance of Z is reached.
2. A handler subclass with a synchronized publish() method is invoked and locks the handler A.
3. toString() is invoked and calls X.foo(), which locks B.
In Thread 2:
1. Code calls X.bar() normally, which locks B and calls Y.baz().
2. Y.baz() invokes the log statement, which goes on to lock the same handler A (not unusual as handlers can be globally shared).
There is now an AB/BA deadlock risk.
None of the code involved in the above example is unreasonable or even unusual. The code doing locking (X) does no logging. The code logging (Y) does no locking, and neither knows that an instance of Z is being logged elsewhere.
Thus, the only place where the deadlock is both known about, and can be mitigated is the log handler itself, which must avoid locking around any callbacks to user code (e.g. toString(), but also via things like the Formattable interface).
Unfortunately StreamHandler uses synchronized on the entire publish() method to protect its write state, and by publishing the method as synchronized, several subclasses follow "best practice" and also synchronize it.
The fix for this bug must involve removing the synchronized keyword from all Handler implementations in the JDK and reworking them to protect their state (where necessary) without synchronizing around any code which may call back to user code.
As such, a CSR will be needed for this issue to explain the situation to users and library developers, especially since any reduction in locking around calls to toString() may cause more concurrent execution of this code and reveal non-thread-safe implementations previously hidden by virtue of being effectively single threaded during logging.
Any fix to this is easily backported if necessary.
MemoryHandler also locks its publish() method, but this class is "special" and has multiple other issues with its design, so I will raise a separate issue to cover this.
- csr for
-
JDK-8349920 j.u.l.Handler classes create deadlock risk via synchronized publish() method
-
- Provisional
-
- relates to
-
JDK-8349208 MemoryHandler logging class can cause incorrect information to be logged.
-
- Open
-
- links to
-
Review(master) openjdk/jdk/23491