[jira] [Commented] (POOL-396) Exceptions in validation can cause an object's lifecycle to be stuck in EVICTION state
[
https://issues.apache.org/jira/browse/POOL-396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17366675#comment-17366675
]
Jeremy Kong commented on POOL-396:
--
No worries! That's good to hear. Yep, rethrowing seems reasonable - I've added
that.
> Exceptions in validation can cause an object's lifecycle to be stuck in
> EVICTION state
> --
>
> Key: POOL-396
> URL: https://issues.apache.org/jira/browse/POOL-396
> Project: Commons Pool
> Issue Type: Bug
>Affects Versions: 2.4.2, 2.9.0
>Reporter: Jeremy Kong
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently, if validation of an object throws an exception when testing an
> idle object during eviction, an exception will be thrown from the evict()
> method. This causes the object to be left in the EVICTION state, but still be
> present in the idleObjects queue.
> Future runs of the eviction thread will run into an infinite loop, because
> they pick up the afflicted object from the queue, fail to set its state to
> EVICTION, don't count it as a test, and repeat ad infinitum. This infinite
> loop means that no further evictions will take place; it also causes
> increases in garbage collection owing to allocation of the EvictionIterator
> on each loop iteration.
> This test added to TestGenericObjectPool (plus a small change allowing
> exceptions to be thrown from the validateObject method of SimpleFactory)
> reproduces the issue.
> {code:java}
> @Test
> @Timeout(value = 6, unit = TimeUnit.MILLISECONDS)
> public void testExceptionInValidationDuringEviction() throws Exception {
> genericObjectPool.setMaxIdle(1);
> genericObjectPool.setMaxTotal(2);
> genericObjectPool.setNumTestsPerEvictionRun(1);
> genericObjectPool.setMinEvictableIdleTime(Duration.ofMillis(0));
> genericObjectPool.setTestWhileIdle(true);
> String active = genericObjectPool.borrowObject();
> genericObjectPool.returnObject(active);
> simpleFactory.setThrowExceptionOnValidate(true);
> try {
> genericObjectPool.evict();
> } catch (Exception e) {
> // OK that a given run fails. But the object should still be evicted.
> }
> genericObjectPool.evict(); // currently fails: causes an infinite loop!
> assertEquals(0, genericObjectPool.getNumActive());
> assertEquals(0, genericObjectPool.getNumIdle());
> }
> {code}
> It seems that protection against exceptions thrown by validation code exist
> elsewhere (e.g. in borrowObject()); similarly, protections against exceptions
> thrown by user eviction policy code exist in evict(). So it looks like the
> testWhileIdle() portion of evict() needs to similarly be protected.
> Thanks!
> Jeremy
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (POOL-396) Exceptions in validation can cause an object's lifecycle to be stuck in EVICTION state
[
https://issues.apache.org/jira/browse/POOL-396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17366264#comment-17366264
]
Phil Steitz commented on POOL-396:
--
Sorry for the slow response. Really nice work on this [~jeremyk-91]. One
small change (responding to your question above) is that I would just rethrow
the original exception rather than wrapping in a NSEE. That makes sense on
borrow, but here it's probably better to just propagate whatever was thrown by
the factory.
> Exceptions in validation can cause an object's lifecycle to be stuck in
> EVICTION state
> --
>
> Key: POOL-396
> URL: https://issues.apache.org/jira/browse/POOL-396
> Project: Commons Pool
> Issue Type: Bug
>Affects Versions: 2.4.2, 2.9.0
>Reporter: Jeremy Kong
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently, if validation of an object throws an exception when testing an
> idle object during eviction, an exception will be thrown from the evict()
> method. This causes the object to be left in the EVICTION state, but still be
> present in the idleObjects queue.
> Future runs of the eviction thread will run into an infinite loop, because
> they pick up the afflicted object from the queue, fail to set its state to
> EVICTION, don't count it as a test, and repeat ad infinitum. This infinite
> loop means that no further evictions will take place; it also causes
> increases in garbage collection owing to allocation of the EvictionIterator
> on each loop iteration.
> This test added to TestGenericObjectPool (plus a small change allowing
> exceptions to be thrown from the validateObject method of SimpleFactory)
> reproduces the issue.
> {code:java}
> @Test
> @Timeout(value = 6, unit = TimeUnit.MILLISECONDS)
> public void testExceptionInValidationDuringEviction() throws Exception {
> genericObjectPool.setMaxIdle(1);
> genericObjectPool.setMaxTotal(2);
> genericObjectPool.setNumTestsPerEvictionRun(1);
> genericObjectPool.setMinEvictableIdleTime(Duration.ofMillis(0));
> genericObjectPool.setTestWhileIdle(true);
> String active = genericObjectPool.borrowObject();
> genericObjectPool.returnObject(active);
> simpleFactory.setThrowExceptionOnValidate(true);
> try {
> genericObjectPool.evict();
> } catch (Exception e) {
> // OK that a given run fails. But the object should still be evicted.
> }
> genericObjectPool.evict(); // currently fails: causes an infinite loop!
> assertEquals(0, genericObjectPool.getNumActive());
> assertEquals(0, genericObjectPool.getNumIdle());
> }
> {code}
> It seems that protection against exceptions thrown by validation code exist
> elsewhere (e.g. in borrowObject()); similarly, protections against exceptions
> thrown by user eviction policy code exist in evict(). So it looks like the
> testWhileIdle() portion of evict() needs to similarly be protected.
> Thanks!
> Jeremy
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (POOL-396) Exceptions in validation can cause an object's lifecycle to be stuck in EVICTION state
[
https://issues.apache.org/jira/browse/POOL-396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17366259#comment-17366259
]
Gary D. Gregory commented on POOL-396:
--
Unless [~psteitz] chimes back in with more suggestions or an answer to your
question, I intent to merge the PR.
> Exceptions in validation can cause an object's lifecycle to be stuck in
> EVICTION state
> --
>
> Key: POOL-396
> URL: https://issues.apache.org/jira/browse/POOL-396
> Project: Commons Pool
> Issue Type: Bug
>Affects Versions: 2.4.2, 2.9.0
>Reporter: Jeremy Kong
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently, if validation of an object throws an exception when testing an
> idle object during eviction, an exception will be thrown from the evict()
> method. This causes the object to be left in the EVICTION state, but still be
> present in the idleObjects queue.
> Future runs of the eviction thread will run into an infinite loop, because
> they pick up the afflicted object from the queue, fail to set its state to
> EVICTION, don't count it as a test, and repeat ad infinitum. This infinite
> loop means that no further evictions will take place; it also causes
> increases in garbage collection owing to allocation of the EvictionIterator
> on each loop iteration.
> This test added to TestGenericObjectPool (plus a small change allowing
> exceptions to be thrown from the validateObject method of SimpleFactory)
> reproduces the issue.
> {code:java}
> @Test
> @Timeout(value = 6, unit = TimeUnit.MILLISECONDS)
> public void testExceptionInValidationDuringEviction() throws Exception {
> genericObjectPool.setMaxIdle(1);
> genericObjectPool.setMaxTotal(2);
> genericObjectPool.setNumTestsPerEvictionRun(1);
> genericObjectPool.setMinEvictableIdleTime(Duration.ofMillis(0));
> genericObjectPool.setTestWhileIdle(true);
> String active = genericObjectPool.borrowObject();
> genericObjectPool.returnObject(active);
> simpleFactory.setThrowExceptionOnValidate(true);
> try {
> genericObjectPool.evict();
> } catch (Exception e) {
> // OK that a given run fails. But the object should still be evicted.
> }
> genericObjectPool.evict(); // currently fails: causes an infinite loop!
> assertEquals(0, genericObjectPool.getNumActive());
> assertEquals(0, genericObjectPool.getNumIdle());
> }
> {code}
> It seems that protection against exceptions thrown by validation code exist
> elsewhere (e.g. in borrowObject()); similarly, protections against exceptions
> thrown by user eviction policy code exist in evict(). So it looks like the
> testWhileIdle() portion of evict() needs to similarly be protected.
> Thanks!
> Jeremy
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (POOL-396) Exceptions in validation can cause an object's lifecycle to be stuck in EVICTION state
[
https://issues.apache.org/jira/browse/POOL-396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17364427#comment-17364427
]
Jeremy Kong commented on POOL-396:
--
I see, that makes sense. Yep, I've updated the PR to do that, and added the
analogous patch for GenericKeyedObjectPool (plus tests).
Would NoSuchElementException be the right exception to throw in this case? I've
copied it from the existing validation failure handling code, though while that
makes sense for e.g. borrowObject() I don't know if it does for evict().
> Exceptions in validation can cause an object's lifecycle to be stuck in
> EVICTION state
> --
>
> Key: POOL-396
> URL: https://issues.apache.org/jira/browse/POOL-396
> Project: Commons Pool
> Issue Type: Bug
>Affects Versions: 2.4.2, 2.9.0
>Reporter: Jeremy Kong
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Currently, if validation of an object throws an exception when testing an
> idle object during eviction, an exception will be thrown from the evict()
> method. This causes the object to be left in the EVICTION state, but still be
> present in the idleObjects queue.
> Future runs of the eviction thread will run into an infinite loop, because
> they pick up the afflicted object from the queue, fail to set its state to
> EVICTION, don't count it as a test, and repeat ad infinitum. This infinite
> loop means that no further evictions will take place; it also causes
> increases in garbage collection owing to allocation of the EvictionIterator
> on each loop iteration.
> This test added to TestGenericObjectPool (plus a small change allowing
> exceptions to be thrown from the validateObject method of SimpleFactory)
> reproduces the issue.
> {code:java}
> @Test
> @Timeout(value = 6, unit = TimeUnit.MILLISECONDS)
> public void testExceptionInValidationDuringEviction() throws Exception {
> genericObjectPool.setMaxIdle(1);
> genericObjectPool.setMaxTotal(2);
> genericObjectPool.setNumTestsPerEvictionRun(1);
> genericObjectPool.setMinEvictableIdleTime(Duration.ofMillis(0));
> genericObjectPool.setTestWhileIdle(true);
> String active = genericObjectPool.borrowObject();
> genericObjectPool.returnObject(active);
> simpleFactory.setThrowExceptionOnValidate(true);
> try {
> genericObjectPool.evict();
> } catch (Exception e) {
> // OK that a given run fails. But the object should still be evicted.
> }
> genericObjectPool.evict(); // currently fails: causes an infinite loop!
> assertEquals(0, genericObjectPool.getNumActive());
> assertEquals(0, genericObjectPool.getNumIdle());
> }
> {code}
> It seems that protection against exceptions thrown by validation code exist
> elsewhere (e.g. in borrowObject()); similarly, protections against exceptions
> thrown by user eviction policy code exist in evict(). So it looks like the
> testWhileIdle() portion of evict() needs to similarly be protected.
> Thanks!
> Jeremy
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (POOL-396) Exceptions in validation can cause an object's lifecycle to be stuck in EVICTION state
[
https://issues.apache.org/jira/browse/POOL-396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17364356#comment-17364356
]
Phil Steitz commented on POOL-396:
--
After sleeping on this, I think it is probably better to go ahead and destroy
the instance before rethrowing the exception. We should also not decrement the
numtests counter in this case.
We should probably add prominent docs somewhere (site and javadoc) pointing out
that factory methods should handle their own exceptions. We try to do
reasonable things when they throw, but this case is a good example where we
need the factory to tell us what to do.
> Exceptions in validation can cause an object's lifecycle to be stuck in
> EVICTION state
> --
>
> Key: POOL-396
> URL: https://issues.apache.org/jira/browse/POOL-396
> Project: Commons Pool
> Issue Type: Bug
>Affects Versions: 2.4.2, 2.9.0
>Reporter: Jeremy Kong
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Currently, if validation of an object throws an exception when testing an
> idle object during eviction, an exception will be thrown from the evict()
> method. This causes the object to be left in the EVICTION state, but still be
> present in the idleObjects queue.
> Future runs of the eviction thread will run into an infinite loop, because
> they pick up the afflicted object from the queue, fail to set its state to
> EVICTION, don't count it as a test, and repeat ad infinitum. This infinite
> loop means that no further evictions will take place; it also causes
> increases in garbage collection owing to allocation of the EvictionIterator
> on each loop iteration.
> This test added to TestGenericObjectPool (plus a small change allowing
> exceptions to be thrown from the validateObject method of SimpleFactory)
> reproduces the issue.
> {code:java}
> @Test
> @Timeout(value = 6, unit = TimeUnit.MILLISECONDS)
> public void testExceptionInValidationDuringEviction() throws Exception {
> genericObjectPool.setMaxIdle(1);
> genericObjectPool.setMaxTotal(2);
> genericObjectPool.setNumTestsPerEvictionRun(1);
> genericObjectPool.setMinEvictableIdleTime(Duration.ofMillis(0));
> genericObjectPool.setTestWhileIdle(true);
> String active = genericObjectPool.borrowObject();
> genericObjectPool.returnObject(active);
> simpleFactory.setThrowExceptionOnValidate(true);
> try {
> genericObjectPool.evict();
> } catch (Exception e) {
> // OK that a given run fails. But the object should still be evicted.
> }
> genericObjectPool.evict(); // currently fails: causes an infinite loop!
> assertEquals(0, genericObjectPool.getNumActive());
> assertEquals(0, genericObjectPool.getNumIdle());
> }
> {code}
> It seems that protection against exceptions thrown by validation code exist
> elsewhere (e.g. in borrowObject()); similarly, protections against exceptions
> thrown by user eviction policy code exist in evict(). So it looks like the
> testWhileIdle() portion of evict() needs to similarly be protected.
> Thanks!
> Jeremy
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (POOL-396) Exceptions in validation can cause an object's lifecycle to be stuck in EVICTION state
[
https://issues.apache.org/jira/browse/POOL-396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17364015#comment-17364015
]
Phil Steitz commented on POOL-396:
--
Nice work on the report and patch. Thanks especially for the unit test
showing the bug. I have a couple of comments on the patch:
# We should remember to apply an analogous patch to GenericKeyedObjectPool,
which is subject to the same issue.
# I think it would be better to fix the eviction state of the object under
test and rethrow the exception (like what borrowObject does when the instance
failing validation has just been created) rather than swallowing the exception
and destroying the instance under test.
When I say "fix the eviction state" I mean basically just call endEvictionTest.
My reasoning for this is that validation exceptions often indicate
infrastructure issues that may be transient and that clients may want to know
about.
> Exceptions in validation can cause an object's lifecycle to be stuck in
> EVICTION state
> --
>
> Key: POOL-396
> URL: https://issues.apache.org/jira/browse/POOL-396
> Project: Commons Pool
> Issue Type: Bug
>Affects Versions: 2.4.2, 2.9.0
>Reporter: Jeremy Kong
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Currently, if validation of an object throws an exception when testing an
> idle object during eviction, an exception will be thrown from the evict()
> method. This causes the object to be left in the EVICTION state, but still be
> present in the idleObjects queue.
> Future runs of the eviction thread will run into an infinite loop, because
> they pick up the afflicted object from the queue, fail to set its state to
> EVICTION, don't count it as a test, and repeat ad infinitum. This infinite
> loop means that no further evictions will take place; it also causes
> increases in garbage collection owing to allocation of the EvictionIterator
> on each loop iteration.
> This test added to TestGenericObjectPool (plus a small change allowing
> exceptions to be thrown from the validateObject method of SimpleFactory)
> reproduces the issue.
> {code:java}
> @Test
> @Timeout(value = 6, unit = TimeUnit.MILLISECONDS)
> public void testExceptionInValidationDuringEviction() throws Exception {
> genericObjectPool.setMaxIdle(1);
> genericObjectPool.setMaxTotal(2);
> genericObjectPool.setNumTestsPerEvictionRun(1);
> genericObjectPool.setMinEvictableIdleTime(Duration.ofMillis(0));
> genericObjectPool.setTestWhileIdle(true);
> String active = genericObjectPool.borrowObject();
> genericObjectPool.returnObject(active);
> simpleFactory.setThrowExceptionOnValidate(true);
> try {
> genericObjectPool.evict();
> } catch (Exception e) {
> // OK that a given run fails. But the object should still be evicted.
> }
> genericObjectPool.evict(); // currently fails: causes an infinite loop!
> assertEquals(0, genericObjectPool.getNumActive());
> assertEquals(0, genericObjectPool.getNumIdle());
> }
> {code}
> It seems that protection against exceptions thrown by validation code exist
> elsewhere (e.g. in borrowObject()); similarly, protections against exceptions
> thrown by user eviction policy code exist in evict(). So it looks like the
> testWhileIdle() portion of evict() needs to similarly be protected.
> Thanks!
> Jeremy
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (POOL-396) Exceptions in validation can cause an object's lifecycle to be stuck in EVICTION state
[
https://issues.apache.org/jira/browse/POOL-396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17363643#comment-17363643
]
Gary D. Gregory commented on POOL-396:
--
[~psteitz]
Any thoughs on the PR?
> Exceptions in validation can cause an object's lifecycle to be stuck in
> EVICTION state
> --
>
> Key: POOL-396
> URL: https://issues.apache.org/jira/browse/POOL-396
> Project: Commons Pool
> Issue Type: Bug
>Affects Versions: 2.4.2, 2.9.0
>Reporter: Jeremy Kong
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Currently, if validation of an object throws an exception when testing an
> idle object during eviction, an exception will be thrown from the evict()
> method. This causes the object to be left in the EVICTION state, but still be
> present in the idleObjects queue.
> Future runs of the eviction thread will run into an infinite loop, because
> they pick up the afflicted object from the queue, fail to set its state to
> EVICTION, don't count it as a test, and repeat ad infinitum. This infinite
> loop means that no further evictions will take place; it also causes
> increases in garbage collection owing to allocation of the EvictionIterator
> on each loop iteration.
> This test added to TestGenericObjectPool (plus a small change allowing
> exceptions to be thrown from the validateObject method of SimpleFactory)
> reproduces the issue.
> {code:java}
> @Test
> @Timeout(value = 6, unit = TimeUnit.MILLISECONDS)
> public void testExceptionInValidationDuringEviction() throws Exception {
> genericObjectPool.setMaxIdle(1);
> genericObjectPool.setMaxTotal(2);
> genericObjectPool.setNumTestsPerEvictionRun(1);
> genericObjectPool.setMinEvictableIdleTime(Duration.ofMillis(0));
> genericObjectPool.setTestWhileIdle(true);
> String active = genericObjectPool.borrowObject();
> genericObjectPool.returnObject(active);
> simpleFactory.setThrowExceptionOnValidate(true);
> try {
> genericObjectPool.evict();
> } catch (Exception e) {
> // OK that a given run fails. But the object should still be evicted.
> }
> genericObjectPool.evict(); // currently fails: causes an infinite loop!
> assertEquals(0, genericObjectPool.getNumActive());
> assertEquals(0, genericObjectPool.getNumIdle());
> }
> {code}
> It seems that protection against exceptions thrown by validation code exist
> elsewhere (e.g. in borrowObject()); similarly, protections against exceptions
> thrown by user eviction policy code exist in evict(). So it looks like the
> testWhileIdle() portion of evict() needs to similarly be protected.
> Thanks!
> Jeremy
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (POOL-396) Exceptions in validation can cause an object's lifecycle to be stuck in EVICTION state
[
https://issues.apache.org/jira/browse/POOL-396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17362952#comment-17362952
]
Jeremy Kong commented on POOL-396:
--
I have proposed a fix in
[https://github.com/apache/commons-pool/pull/85|https://github.com/apache/commons-pool/pull/85].
> Exceptions in validation can cause an object's lifecycle to be stuck in
> EVICTION state
> --
>
> Key: POOL-396
> URL: https://issues.apache.org/jira/browse/POOL-396
> Project: Commons Pool
> Issue Type: Bug
>Affects Versions: 2.4.2, 2.9.0
>Reporter: Jeremy Kong
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently, if validation of an object throws an exception when testing an
> idle object during eviction, an exception will be thrown from the evict()
> method. This causes the object to be left in the EVICTION state, but still be
> present in the idleObjects queue.
> Future runs of the eviction thread will run into an infinite loop, because
> they pick up the afflicted object from the queue, fail to set its state to
> EVICTION, don't count it as a test, and repeat ad infinitum. This infinite
> loop means that no further evictions will take place; it also causes
> increases in garbage collection owing to allocation of the EvictionIterator
> on each loop iteration.
> This test added to TestGenericObjectPool (plus a small change allowing
> exceptions to be thrown from the validateObject method of SimpleFactory)
> reproduces the issue.
> {code:java}
> @Test
> @Timeout(value = 6, unit = TimeUnit.MILLISECONDS)
> public void testExceptionInValidationDuringEviction() throws Exception {
> genericObjectPool.setMaxIdle(1);
> genericObjectPool.setMaxTotal(2);
> genericObjectPool.setNumTestsPerEvictionRun(1);
> genericObjectPool.setMinEvictableIdleTime(Duration.ofMillis(0));
> genericObjectPool.setTestWhileIdle(true);
> String active = genericObjectPool.borrowObject();
> genericObjectPool.returnObject(active);
> simpleFactory.setThrowExceptionOnValidate(true);
> try {
> genericObjectPool.evict();
> } catch (Exception e) {
> // OK that a given run fails. But the object should still be evicted.
> }
> genericObjectPool.evict(); // currently fails: causes an infinite loop!
> assertEquals(0, genericObjectPool.getNumActive());
> assertEquals(0, genericObjectPool.getNumIdle());
> }
> {code}
> It seems that protection against exceptions thrown by validation code exist
> elsewhere (e.g. in borrowObject()); similarly, protections against exceptions
> thrown by user eviction policy code exist in evict(). So it looks like the
> testWhileIdle() portion of evict() needs to similarly be protected.
> Thanks!
> Jeremy
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
