rmannibucau commented on a change in pull request #68:
URL: https://github.com/apache/commons-dbcp/pull/68#discussion_r498247272
##########
File path: src/main/java/org/apache/commons/dbcp2/PoolableConnectionFactory.java
##########
@@ -94,6 +100,8 @@
private Integer defaultQueryTimeoutSeconds;
+ private final ExecutorService executor = Executors.newFixedThreadPool(3,
new AbortThreadFactory());
Review comment:
Why not making it synchronous? (Executor e = Runnable::run)
In this state it can fail at instantiation if system already uses a lot of
threads (and leak), leak at shutdown/undeploy time (an issue for OSGi and
application servers) due to daemon thread usage and the lack of await at close
time.
If really desired to be thread based, minimum for me is to 1. await the stop
properly (= handle the lifecycle completely and not assume it will be done fast
after close() is called since quickly after close the classloader can be
destroyed and finalization can fail), 2. make the executor lazy (if no
connection are abandonned it is pointless to consume these resources and to
slow down the instantiation/startup)
That said I'm really to make it synchronous since the async there just
brings issues IMHO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
