[GitHub] commons-pool pull request #17: POOL-359: prevent NPE closing multiple Generi...

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

[GitHub] commons-pool pull request #17: POOL-359: prevent NPE closing multiple Generi...

RahulNagekar
GitHub user mswintermeyer opened a pull request:

    https://github.com/apache/commons-pool/pull/17

    POOL-359: prevent NPE closing multiple GenericObjectPool's

   

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

    $ git pull https://github.com/mswintermeyer/commons-pool fixNPEClosePools

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

    https://github.com/apache/commons-pool/pull/17.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 #17
   
----
commit 0fdde72475b378cbd0fb707d52385196d928a1da
Author: Michael Wintermeyer <michaelw@...>
Date:   2018-11-16T20:23:51Z

    POOL-359: prevent NPE closing multiple GenericObjectPool's

----


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool issue #17: POOL-359: prevent NPE closing multiple GenericObject...

RahulNagekar
Github user mswintermeyer commented on the issue:

    https://github.com/apache/commons-pool/pull/17
 
    If `cancel` should not be called more than once, then I believe that would require a greater re-architecture. `GenericObjectPool`'s would need to keep track of whether another object pool called `cancel`: to rely on something other than non-static state (`closed` variable on `BaseGenericObjectPool`) to call a static method. To me, it seems cleanest for static methods to be designed defensively. So I'm not aware of a reason to not allow this, but I acknowledge I'm new to the codebase.
   
    Side note that the risk of an NPE in `cancel` seems somewhat new. [This commit](https://github.com/apache/commons-pool/commit/867614829ba6b041ca6becbad0f43cb180003404#diff-38e254894b87bdf9a1758778c7ffd50fL96) refactored `EvictionTimer` to depend on the executor's queue rather than an integer `usageCount`.


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool issue #17: POOL-359: prevent NPE closing multiple GenericObject...

RahulNagekar
In reply to this post by RahulNagekar
Github user grimreaper commented on the issue:

    https://github.com/apache/commons-pool/pull/17
 
    Works for me. Blocked on INFRA-17267
    travis fails for unrelated reasons, but tests pass locally:
    ```
    [INFO] ------------------------------------------------------------------------
    [INFO] BUILD SUCCESS
    [INFO] ------------------------------------------------------------------------
    [INFO] Total time:  05:45 min
    [INFO] Finished at: 2018-11-18T20:54:44-08:00
    [INFO] ------------------------------------------------------------------------
    ```


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool issue #17: POOL-359: prevent NPE closing multiple GenericObject...

RahulNagekar
In reply to this post by RahulNagekar
Github user graben commented on the issue:

    https://github.com/apache/commons-pool/pull/17
 
    #18 fixes actual failing travis build!


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool issue #17: POOL-359: prevent NPE closing multiple GenericObject...

RahulNagekar
In reply to this post by RahulNagekar
Github user mswintermeyer commented on the issue:

    https://github.com/apache/commons-pool/pull/17
 
    @garydgregory the test was failing on my machine without the main code change, so thanks for flagging that it was not consistent! I've updated the test, and I believe this time it's a consistent failure :)


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool issue #17: POOL-359: prevent NPE closing multiple GenericObject...

RahulNagekar
In reply to this post by RahulNagekar
Github user mswintermeyer commented on the issue:

    https://github.com/apache/commons-pool/pull/17
 
    Also, thank you for merging in the meantime, as well as for contributing your time to maintain the project!


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool issue #17: POOL-359: prevent NPE closing multiple GenericObject...

RahulNagekar
In reply to this post by RahulNagekar
Github user grimreaper commented on the issue:

    https://github.com/apache/commons-pool/pull/17
 
    The new test doesn't fail for me if I reverse the original change:
    ```
    [INFO] -------------------------------------------------------
    [INFO]  T E S T S
    [INFO] -------------------------------------------------------
    [INFO] Running org.apache.commons.pool2.impl.TestGenericObjectPool
    [INFO] Tests run: 84, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 106.528 s - in org.apache.commons.pool2.impl.TestGenericObjectPool
    [INFO]
    [INFO] Results:
    [INFO]
    [INFO] Tests run: 84, Failures: 0, Errors: 0, Skipped: 0
    [INFO]
    [INFO] ------------------------------------------------------------------------
    [INFO] BUILD SUCCESS
    [INFO] ------------------------------------------------------------------------
    [INFO] Total time:  01:57 min
    [INFO] Finished at: 2018-11-20T22:58:11-08:00
    [INFO] ------------------------------------------------------------------------
    mvn test -Dtest=TestGenericObjectPool 34.10s user 7.66s system 34% cpu 1:59.76 total; max RSS 305800Ki
    [45682 22:58:11.583 eax@FlyingEagle ...ache/commons/commons-pool]∴git diff           (git:commons-pool)-[master●]
    diff --git i/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java w/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java
    index b92b87a9..2fc9ffd8 100644
    --- i/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java
    +++ w/src/main/java/org/apache/commons/pool2/impl/EvictionTimer.java
    @@ -94,7 +94,7 @@ static synchronized void schedule(
         static synchronized void cancel(
                 final BaseGenericObjectPool<?>.Evictor task, final long timeout, final TimeUnit unit) {
             task.cancel();
    -        if (executor != null && executor.getQueue().size() == 0) {
    +        if (executor.getQueue().size() == 0) {
                 executor.shutdown();
                 try {
                     executor.awaitTermination(timeout, unit);
    ```


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool issue #17: POOL-359: prevent NPE closing multiple GenericObject...

RahulNagekar
In reply to this post by RahulNagekar
Github user grimreaper commented on the issue:

    https://github.com/apache/commons-pool/pull/17
 
    hrm... or at least its not consistent. I just ran it twice more and failed only once.
    ∴mvn test -Dtest=TestGenericObjectPool#testCloseMultiplePools
   
    I'm not generally the biggest fan of tests that don't rely on a fixed clock: you're guaranteed to eventually get a failure.  


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool issue #17: POOL-359: prevent NPE closing multiple GenericObject...

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

    https://github.com/apache/commons-pool/pull/17
 
    I think this PR can be closed.


---

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

Reply | Threaded
Open this post in threaded view
|

[GitHub] commons-pool pull request #17: POOL-359: prevent NPE closing multiple Generi...

RahulNagekar
In reply to this post by RahulNagekar
Github user mswintermeyer closed the pull request at:

    https://github.com/apache/commons-pool/pull/17


---

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