[GitHub] commons-dbutils pull request #3: Fixes a thread safety problem introduced by...

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-dbutils pull request #3: Fixes a thread safety problem introduced by...

testingsavvy
GitHub user hdevalke opened a pull request:

    https://github.com/apache/commons-dbutils/pull/3

    Fixes a thread safety problem introduced by DBUTILS-124.

    ColumnHandlers and PropertyHandlers are preloaded in a list as the ServiceLoader instances are not thread safe

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/hdevalke/commons-dbutils master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/commons-dbutils/pull/3.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #3
   
----
commit 59d114082708ade64c9f05b13128d0c0eb39bc7f
Author: Hannes De Valkeneer <[hidden email]>
Date:   2017-10-05T14:05:18Z

    Fixes a thread safety problem introduced by DBUTILS-124.
   
    ColumnHandlers and PropertyHandlers are preloaded in a list as the ServiceLoader instances are not thread safe

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-dbutils issue #3: Fixes a thread safety problem introduced by DBUTIL...

testingsavvy
Github user thecarlhall commented on the issue:

    https://github.com/apache/commons-dbutils/pull/3
 
    Thanks, @hdevalke!  Nice catch.  I totally missed that `ServiceLoader` isn't thread safe.  I'll work to get this merged into master and cut a new release as soon as I can.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-dbutils pull request #3: Fixes a thread safety problem introduced by...

testingsavvy
In reply to this post by testingsavvy
Github user garydgregory commented on a diff in the pull request:

    https://github.com/apache/commons-dbutils/pull/3#discussion_r143814271
 
    --- Diff: src/main/java/org/apache/commons/dbutils/BeanProcessor.java ---
    @@ -65,19 +65,21 @@
          */
         private static final Map<Class<?>, Object> primitiveDefaults = new HashMap<Class<?>, Object>();
     
    -    /**
    -     * ServiceLoader to find <code>ColumnHandler</code> implementations on the classpath.  The iterator for this is
    -     * lazy and each time <code>iterator()</code> is called.
    -     */
    -    // FIXME: I think this instantiates new handlers on each iterator() call. This might be worth caching upfront.
    -    private static final ServiceLoader<ColumnHandler> columnHandlers = ServiceLoader.load(ColumnHandler.class);
    +    private static final List<ColumnHandler> columnHandlers = new ArrayList<ColumnHandler>();
     
    -    /**
    -     * ServiceLoader to find <code>PropertyHandler</code> implementations on the classpath.  The iterator for this is
    -     * lazy and each time <code>iterator()</code> is called.
    -     */
    -    // FIXME: I think this instantiates new handlers on each iterator() call. This might be worth caching upfront.
    -    private static final ServiceLoader<PropertyHandler> propertyHandlers = ServiceLoader.load(PropertyHandler.class);
    +    static{
    +        for (ColumnHandler h : ServiceLoader.load(ColumnHandler.class)) {
    +            columnHandlers.add(h);
    +        }
    +    }
    +
    +    private static final List<PropertyHandler> propertyHandlers = new ArrayList<PropertyHandler>();
    +
    +    static {
    +        for (PropertyHandler h : ServiceLoader.load(PropertyHandler.class)) {
    +            propertyHandlers.add(h);
    --- End diff --
   
    What not use a single static block?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-dbutils pull request #3: Fixes a thread safety problem introduced by...

testingsavvy
In reply to this post by testingsavvy
Github user hdevalke commented on a diff in the pull request:

    https://github.com/apache/commons-dbutils/pull/3#discussion_r143818665
 
    --- Diff: src/main/java/org/apache/commons/dbutils/BeanProcessor.java ---
    @@ -65,19 +65,21 @@
          */
         private static final Map<Class<?>, Object> primitiveDefaults = new HashMap<Class<?>, Object>();
     
    -    /**
    -     * ServiceLoader to find <code>ColumnHandler</code> implementations on the classpath.  The iterator for this is
    -     * lazy and each time <code>iterator()</code> is called.
    -     */
    -    // FIXME: I think this instantiates new handlers on each iterator() call. This might be worth caching upfront.
    -    private static final ServiceLoader<ColumnHandler> columnHandlers = ServiceLoader.load(ColumnHandler.class);
    +    private static final List<ColumnHandler> columnHandlers = new ArrayList<ColumnHandler>();
     
    -    /**
    -     * ServiceLoader to find <code>PropertyHandler</code> implementations on the classpath.  The iterator for this is
    -     * lazy and each time <code>iterator()</code> is called.
    -     */
    -    // FIXME: I think this instantiates new handlers on each iterator() call. This might be worth caching upfront.
    -    private static final ServiceLoader<PropertyHandler> propertyHandlers = ServiceLoader.load(PropertyHandler.class);
    +    static{
    +        for (ColumnHandler h : ServiceLoader.load(ColumnHandler.class)) {
    +            columnHandlers.add(h);
    +        }
    +    }
    +
    +    private static final List<PropertyHandler> propertyHandlers = new ArrayList<PropertyHandler>();
    +
    +    static {
    +        for (PropertyHandler h : ServiceLoader.load(PropertyHandler.class)) {
    +            propertyHandlers.add(h);
    --- End diff --
   
    Can you explain why that is problematic?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-dbutils pull request #3: Fixes a thread safety problem introduced by...

testingsavvy
In reply to this post by testingsavvy
Github user garydgregory commented on a diff in the pull request:

    https://github.com/apache/commons-dbutils/pull/3#discussion_r143822953
 
    --- Diff: src/main/java/org/apache/commons/dbutils/BeanProcessor.java ---
    @@ -65,19 +65,21 @@
          */
         private static final Map<Class<?>, Object> primitiveDefaults = new HashMap<Class<?>, Object>();
     
    -    /**
    -     * ServiceLoader to find <code>ColumnHandler</code> implementations on the classpath.  The iterator for this is
    -     * lazy and each time <code>iterator()</code> is called.
    -     */
    -    // FIXME: I think this instantiates new handlers on each iterator() call. This might be worth caching upfront.
    -    private static final ServiceLoader<ColumnHandler> columnHandlers = ServiceLoader.load(ColumnHandler.class);
    +    private static final List<ColumnHandler> columnHandlers = new ArrayList<ColumnHandler>();
     
    -    /**
    -     * ServiceLoader to find <code>PropertyHandler</code> implementations on the classpath.  The iterator for this is
    -     * lazy and each time <code>iterator()</code> is called.
    -     */
    -    // FIXME: I think this instantiates new handlers on each iterator() call. This might be worth caching upfront.
    -    private static final ServiceLoader<PropertyHandler> propertyHandlers = ServiceLoader.load(PropertyHandler.class);
    +    static{
    +        for (ColumnHandler h : ServiceLoader.load(ColumnHandler.class)) {
    +            columnHandlers.add(h);
    +        }
    +    }
    +
    +    private static final List<PropertyHandler> propertyHandlers = new ArrayList<PropertyHandler>();
    +
    +    static {
    +        for (PropertyHandler h : ServiceLoader.load(PropertyHandler.class)) {
    +            propertyHandlers.add(h);
    --- End diff --
   
    I think of it the other way around: Why use two blocks when you could use one?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-dbutils pull request #3: Fixes a thread safety problem introduced by...

testingsavvy
In reply to this post by testingsavvy
Github user hdevalke commented on a diff in the pull request:

    https://github.com/apache/commons-dbutils/pull/3#discussion_r143833029
 
    --- Diff: src/main/java/org/apache/commons/dbutils/BeanProcessor.java ---
    @@ -65,19 +65,21 @@
          */
         private static final Map<Class<?>, Object> primitiveDefaults = new HashMap<Class<?>, Object>();
     
    -    /**
    -     * ServiceLoader to find <code>ColumnHandler</code> implementations on the classpath.  The iterator for this is
    -     * lazy and each time <code>iterator()</code> is called.
    -     */
    -    // FIXME: I think this instantiates new handlers on each iterator() call. This might be worth caching upfront.
    -    private static final ServiceLoader<ColumnHandler> columnHandlers = ServiceLoader.load(ColumnHandler.class);
    +    private static final List<ColumnHandler> columnHandlers = new ArrayList<ColumnHandler>();
     
    -    /**
    -     * ServiceLoader to find <code>PropertyHandler</code> implementations on the classpath.  The iterator for this is
    -     * lazy and each time <code>iterator()</code> is called.
    -     */
    -    // FIXME: I think this instantiates new handlers on each iterator() call. This might be worth caching upfront.
    -    private static final ServiceLoader<PropertyHandler> propertyHandlers = ServiceLoader.load(PropertyHandler.class);
    +    static{
    +        for (ColumnHandler h : ServiceLoader.load(ColumnHandler.class)) {
    +            columnHandlers.add(h);
    +        }
    +    }
    +
    +    private static final List<PropertyHandler> propertyHandlers = new ArrayList<PropertyHandler>();
    +
    +    static {
    +        for (PropertyHandler h : ServiceLoader.load(PropertyHandler.class)) {
    +            propertyHandlers.add(h);
    --- End diff --
   
    I didn't see there was already a static block initializing `primitiveDefaults`. I find it more readable initializing each static variable in their own block. I did an update and put all in one static block.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-dbutils issue #3: Fixes a thread safety problem introduced by DBUTIL...

testingsavvy
In reply to this post by testingsavvy
Github user garydgregory commented on the issue:

    https://github.com/apache/commons-dbutils/pull/3
 
    I created https://issues.apache.org/jira/browse/DBUTILS-135 to track this fix. Committed to git master.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]