TimeoutTimer thread pool max

[shayaantx]

Hello,

I noticed the TimeoutTimer in older versions started a thread for every query that had a query timeout greater than 0. Then I saw in #150 someone had reported it. #162 fixed it.

Not sure if I'm missing something, but how is this problem fixed? In TimeoutTimer the executor service is using a cached thread pool via java's helper methods in ExecutorService. This creates a thread pool with a core pool size of 0 and a max pool size of Integer.MAX. If I'm reading that correctly, if I spawn 1k queries at once with timeouts, I will have 1k additional timeout threads (since no max on the thread pool exists). Seems like the problem still exists, you've just capped the max number of threads that could get created from unlimited to Integer.MAX by using a thread pool.

How come we can't do something like what the oracle jdbc driver does? They have one thread that all queries (that have a timeout) register with and that thread polls every second checking if they should interrupt each thread running the query?

Forgive me if I'm missing something with the original proposed fix

[ulvii]

Hi @shayaantx ,

Please provide a repro code, we will investigate and get back to you.

[shayaantx]

I can attempt to write some reproducible code, but I mean I think the latest code highlights the issue I described.

In the TDSCommand and TDSReader objects, you guys use the class TimeoutTimer (if users of the driver set a query timeout greater than 0). This class has two methods that use a static final ExecutorService that is created once via Executors.newCachedThreadPool(new ThreadFactory()). That specific implementation makes a thread pool that has a max size of Integer.MAX_VALUE, this type of thread pool will reuse threads if they are available, but since the max is such a large number you can run the pool up as many threads as you need.

So if I run 5k queries at once with 5k timeouts configured, depending on how fast each query executes, you could have 10k threads in use, instead of 5k. Obviously this is an extreme case, but the same problem would appear with 500 simultaneous queries configured with timeouts (which in my use cases, is normal amount of queries to run in parallel).

The design here of the query timeout logic seems inefficient. You are creating one timer thread for every query being run at any given time with a query timeout. Oracle doesn't have this problem, mysql has the same issue (https://github.com/mysql/mysql-connector-j/blob/1f61b0b0270d9844b006572ba4e77f19c0f230d4/src/com/mysql/jdbc/StatementImpl.java#L67).

I understand timeouts may be rarely used, but still seems like something worth fixing. The only workaround we implemented was just setting keep alive timeouts at the OS level.

[ulvii]

Hi @shayaantx ,

I looked into this a bit and agree that in a multi-threaded environment (where every thread has its own connection) each query with timeout creates a new thread. I will label this as an enhancement, we will investigate what needs to be done to improve.

[shayaantx]

@ulvii Appreciate it

If I wanted to enhance it myself would you guys consider adding it to the main src?

[ulvii]

Hi @shayaantx ,

Definitely, we appreciate contributions from the community. Please create a PR against the dev branch, the team will get it reviewed.

[ulvii]

#782 is merged.