You are viewing a plain text version of this content. The canonical link for it is here.

Posted to log4j-dev@logging.apache.org by Gary Gregory <ga...@gmail.com> on 2014/08/12 20:53:43 UTC

RE: svn commit: r1617397 - /logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java

Hi,

What is the justification for this extra level?

Gary

<div>-------- Original message --------</div><div>From: mattsicker@apache.org </div><div>Date:08/11/2014  22:04  (GMT-05:00) </div><div>To: commits@logging.apache.org </div><div>Subject: svn commit: r1617397 -
  /logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java </div><div>
</div>Author: mattsicker
Date: Tue Aug 12 02:04:38 2014
New Revision: 1617397

URL: http://svn.apache.org/r1617397
Log:
Singleton pattern

Modified:
    logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java

Modified: logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
URL: http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java?rev=1617397&r1=1617396&r2=1617397&view=diff
==============================================================================
--- logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java (original)
+++ logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java Tue Aug 12 02:04:38 2014
@@ -36,7 +36,6 @@ import org.apache.logging.log4j.core.uti
  * An {@link AbstractDatabaseManager} implementation for relational databases accessed via JDBC.
  */
public final class JdbcDatabaseManager extends AbstractDatabaseManager {
-    private static final JDBCDatabaseManagerFactory FACTORY = new JDBCDatabaseManagerFactory();

     private final List<Column> columns;
     private final ConnectionSource connectionSource;
@@ -174,10 +173,19 @@ public final class JdbcDatabaseManager e
                                                              final ColumnConfig[] columnConfigs) {

         return AbstractDatabaseManager.getManager(
-                name, new FactoryData(bufferSize, connectionSource, tableName, columnConfigs), FACTORY
+                name, new FactoryData(bufferSize, connectionSource, tableName, columnConfigs), getFactory()
         );
     }

+    // the usual lazy singleton
+    private static class Holder {
+        private static final JDBCDatabaseManagerFactory INSTANCE = new JDBCDatabaseManagerFactory();
+    }
+
+    private static JDBCDatabaseManagerFactory getFactory() {
+        return Holder.INSTANCE;
+    }
+
     /**
      * Encapsulates data that {@link JDBCDatabaseManagerFactory} uses to create managers.
      */

Re: svn commit: r1617397 - /logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java

Posted by Matt Sicker <bo...@gmail.com>.

It's a rare problem, but if two threads attempt to initialize a class for
the first time, two instances of the singleton may end up being created.
However, using this pattern, the only possible access point to create the
singleton is through the Holder class which will only be called once in
this pattern.


On 12 August 2014 15:13, Ralph Goers <ra...@dslextreme.com> wrote:

> What race condition?  Static variables are initialized when the class is
> constructed.  As far as I know there is no race condition on that or Java
> would be hosed.
>
> Ralph
>
> On Aug 12, 2014, at 12:07 PM, Matt Sicker <bo...@gmail.com> wrote:
>
> Prevents multiple threads from constructing the field if they access the
> class concurrently. Basically, it prevents a race condition. The
> alternative is to make the field volatile and do a double-checked lock
> which we do in another class somewhere.
>
>
> On 12 August 2014 13:53, Gary Gregory <ga...@gmail.com> wrote:
>
>> Hi,
>>
>> What is the justification for this extra level?
>>
>> Gary
>>
>>
>> -------- Original message --------
>> From: mattsicker@apache.org
>> Date:08/11/2014 22:04 (GMT-05:00)
>> To: commits@logging.apache.org
>> Subject: svn commit: r1617397 -
>> /logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>>
>>
>> Author: mattsicker
>> Date: Tue Aug 12 02:04:38 2014
>> New Revision: 1617397
>>
>> URL: http://svn.apache.org/r1617397
>> Log:
>> Singleton pattern
>>
>> Modified:
>>
>> logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>>
>> Modified:
>> logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>> URL:
>> http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java?rev=1617397&r1=1617396&r2=1617397&view=diff
>>
>> ==============================================================================
>> ---
>> logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>> (original)
>> +++
>> logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>> Tue Aug 12 02:04:38 2014
>> @@ -36,7 +36,6 @@ import org.apache.logging.log4j.core.uti
>>   * An {@link AbstractDatabaseManager} implementation for relational
>> databases accessed via JDBC.
>>   */
>> public final class JdbcDatabaseManager extends AbstractDatabaseManager {
>> -    private static final JDBCDatabaseManagerFactory FACTORY = new
>> JDBCDatabaseManagerFactory();
>>
>>      private final List<Column> columns;
>>      private final ConnectionSource connectionSource;
>> @@ -174,10 +173,19 @@ public final class JdbcDatabaseManager e
>>                                                               final
>> ColumnConfig[] columnConfigs) {
>>
>>          return AbstractDatabaseManager.getManager(
>> -                name, new FactoryData(bufferSize, connectionSource,
>> tableName, columnConfigs), FACTORY
>> +                name, new FactoryData(bufferSize, connectionSource,
>> tableName, columnConfigs), getFactory()
>>          );
>>      }
>>
>> +    // the usual lazy singleton
>> +    private static class Holder {
>> +        private static final JDBCDatabaseManagerFactory INSTANCE = new
>> JDBCDatabaseManagerFactory();
>> +    }
>> +
>> +    private static JDBCDatabaseManagerFactory getFactory() {
>> +        return Holder.INSTANCE;
>> +    }
>> +
>>      /**
>>       * Encapsulates data that {@link JDBCDatabaseManagerFactory} uses to
>> create managers.
>>       */
>>
>>
>>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
>
>


-- 
Matt Sicker <bo...@gmail.com>

Re: RE: svn commit: r1617397 - /logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java

Posted by Gary Gregory <ga...@gmail.com>.

On Wed, Aug 13, 2014 at 1:31 PM, Matt Sicker <bo...@gmail.com> wrote:

>
> On Wednesday, 13 August 2014, Ralph Goers <ra...@dslextreme.com>
> wrote:
>
>> So are you going to change this back?
>>
>> Yeah I'll revert it unless someone else snags it first. I won't be at a
> computer for several hours, though.
>

Matt, please do undo when you do get a chance.

Gary

>
> I would note that this is my relic of old Java development. Similar to the
> calls of size() > 0 instead of isEmpty() you still find all over the place.
>
>
>> Ralph
>>
>> On Aug 12, 2014, at 5:44 PM, Matt Sicker <bo...@gmail.com> wrote:
>>
>> Oh my bad, I mixed up lazy singleton and eager singleton.
>>
>>
>> On 12 August 2014 17:43, Ralph Goers <ra...@dslextreme.com> wrote:
>>
>>> Finally, see
>>> http://en.wikipedia.org/wiki/Singleton_pattern#Eager_initialization.
>>>  This was the prior pattern which is clearly thread safe.
>>>
>>> Ralph
>>>
>>> On Aug 12, 2014, at 3:40 PM, Ralph Goers <ra...@dslextreme.com>
>>> wrote:
>>>
>>> Let me put it another way.
>>>
>>> If your intent is that the factory should only be created when
>>> getManager is called vs when AbstractDataManager is loaded then your change
>>> is fine.  However, I am not sure there is any benefit to that since I would
>>> expect every access to AbstractDataManager to be immediately followed by a
>>> call to getManager.
>>>
>>> From what I can see the previous code was not vulnerable to any race
>>> conditions.
>>>
>>> Ralph
>>>
>>> On Aug 12, 2014, at 3:33 PM, Ralph Goers <ra...@dslextreme.com>
>>> wrote:
>>>
>>> So you are saying static initializers in a class can be executed
>>> multiple times?  If so I certainly wasn’t aware of that and I would think a
>>> few pieces of code would need modification.  Before I go searching can you
>>> point to something that says it works that way?
>>>
>>> When I read
>>> http://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom it
>>> indicates that static initialization IS guaranteed to be serial, in which
>>> case the holder pattern is not needed for this.  That pattern is for when
>>> you want lazy initialization, not for guaranteeing a static variable is
>>> only initialized once.
>>>
>>> Ralph
>>>
>>> On Aug 12, 2014, at 1:13 PM, Ralph Goers <ra...@dslextreme.com>
>>> wrote:
>>>
>>> What race condition?  Static variables are initialized when the class is
>>> constructed.  As far as I know there is no race condition on that or Java
>>> would be hosed.
>>>
>>> Ralph
>>>
>>> On Aug 12, 2014, at 12:07 PM, Matt Sicker <bo...@gmail.com> wrote:
>>>
>>> Prevents multiple threads from constructing the field if they access the
>>> class concurrently. Basically, it prevents a race condition. The
>>> alternative is to make the field volatile and do a double-checked lock
>>> which we do in another class somewhere.
>>>
>>>
>>> On 12 August 2014 13:53, Gary Gregory <ga...@gmail.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> What is the justification for this extra level?
>>>>
>>>> Gary
>>>>
>>>>
>>>> -------- Original message --------
>>>> From: mattsicker@apache.org
>>>> Date:08/11/2014 22:04 (GMT-05:00)
>>>> To: commits@logging.apache.org
>>>> Subject: svn commit: r1617397 -
>>>> /logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>>>>
>>>>
>>>> Author: mattsicker
>>>> Date: Tue Aug 12 02:04:38 2014
>>>> New Revision: 1617397
>>>>
>>>> URL: http://svn.apache.org/r1617397
>>>> Log:
>>>> Singleton pattern
>>>>
>>>> Modified:
>>>>
>>>> logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>>>>
>>>> Modified:
>>>> logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java?rev=1617397&r1=1617396&r2=1617397&view=diff
>>>>
>>>> ==============================================================================
>>>> ---
>>>> logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>>>> (original)
>>>> +++
>>>> logging/log4j/log4j2/trunk/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcDatabaseManager.java
>>>> Tue Aug 12 02:04:38 2014
>>>> @@ -36,7 +36,6 @@ import org.apache.logging.log4j.core.uti
>>>>   * An {@link AbstractDatabaseManager} implementation for relational
>>>> databases accessed via JDBC.
>>>>   */
>>>> public final class JdbcDatabaseManager extends AbstractDatabaseManager {
>>>> -    private static final JDBCDatabaseManagerFactory FACTORY = new
>>>> JDBCDatabaseManagerFactory();
>>>>
>>>>      private final List<Column> columns;
>>>>      private final ConnectionSource connectionSource;
>>>> @@ -174,10 +173,19 @@ public final class JdbcDatabaseManager e
>>>>                                                               final
>>>> ColumnConfig[] columnConfigs) {
>>>>
>>>>          return AbstractDatabaseManager.getManager(
>>>> -                name, new FactoryData(bufferSize, connectionSource,
>>>> tableName, columnConfigs), FACTORY
>>>> +                name, new FactoryData(bufferSize, connectionSource,
>>>> tableName, columnConfigs), getFactory()
>>>>          );
>>>>      }
>>>>
>>>> +    // the usual lazy singleton
>>>> +    private static class Holder {
>>>> +        private static final JDBCDatabaseManagerFactory INSTANCE = new
>>>> JDBCDatabaseManagerFactory();
>>>> +    }
>>>> +
>>>> +    private static JDBCDatabaseManagerFactory getFactory() {
>>>> +        return Holder.INSTANCE;
>>>> +    }
>>>> +
>>>>      /**
>>>>       * Encapsulates data that {@link JDBCDatabaseManagerFactory} uses
>>>> to create managers.
>>>>       */
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Matt Sicker <bo...@gmail.com>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>>
>>
>
> --
> Matt Sicker <bo...@gmail.com>
>



-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory