UV_THREADPOOL_SIZE setting for Oracle / libuv issue

[cjbj]

Environment

Knex version: GitHub master
Database + version: Oracle
OS: all

Bug

An attempt to set the libuv UV_THREADPOOL_SIZE in the code for the Oracle dialect is in

knex/lib/dialects/oracledb/index.js

Lines 18 to 26 in 80355e4

	function Client_Oracledb() {
	Client_Oracle.apply(this, arguments);
	// Node.js only have 4 background threads by default, oracledb needs one by connection
	if (this.driver) {
	process.env.UV_THREADPOOL_SIZE = process.env.UV_THREADPOOL_SIZE || 1;
	process.env.UV_THREADPOOL_SIZE =
	parseInt(process.env.UV_THREADPOOL_SIZE) + this.driver.poolMax;
	}
	}

This setting (i) needs to be made before the apps threadpool is started, so this function looks to be too late. (ii) won't work on Windows for some reason known to Node.js or libuv. The environment variable needs to be set before Node.js starts, eg. in a package.json 'start' script.

Also I could quibble over

knex/CHANGELOG.md

Line 869 in 7cfb78d

- Read oracle's UV_THREADPOOL_SIZE env variable correctly #2372

which refers the UV_THREADPOOL_SIZE as "Oracle's". It is actually Node.js's or technically libuvs.

I don't have a good solution - other than documentation.

[cjbj]

Tagging @atiertant per the issue template.

[kibertoad]

@cjbj So what happens currently if user correctly sets this value in his start script, and then we override it within our code? Does that break things? Also is there any documentation for recommended values for using Oracle that we could refer to from our documentation?

[cjbj]

As far as I can tell from the libuv code, UV_THREADPOOL_SIZE is only ever checked once during thread pool initialization, so overriding it inside Knex won't have an effect unless it is done before the thread pool starts. Also overriding it is known not to work on Windows; I don't know why. One problem with overriding it is that it makes it hard to see what size was actually used, since there is no way (?) to get the actual pool size from Node.js. The env var is the next best thing for logging the size but can be 'wrong' if the var was set after the pool started.

The node-oracledb doc about this is at Connections, Threads, and Parallelism.

[kibertoad]

@cjbj Thank! I'll update the docs.

[atiertant]

@cjbj @kibertoad in our tests, if UV_THREADPOOL_SIZE wasn't set correctly, some transaction never finish without any error.
maybe driver has change...
see oracle/node-oracledb#395

[aidenfoxx]

@atiertant I also experienced this. And the suggestion in that thread about releasing the connection doesn't help.

For better debugging I replaced the tarn pool with the default oracle pool for the oracledb dialect, with explicit connection releasing whenever anything finishes. The connections would still lock up as soon as the pool exceeded the UV_THREADPOOL_SIZE.

Feels to me like this is an issue with node-oracledb, and the best we can do is throw a warning to the user when the pool size is greater than the UV_THREADPOOL_SIZE, since changing UV_THREADPOOL_SIZE doesn't work.

[cjbj]

@aidenfoxx @atiertant You definitely need UV_THREADPOOL_SIZE set correctly - at the right place and right value. Some users have (had) it quite high to cope with code that tries constructs like Promise.all() on a single connection. In node-oracledb 5.2 the execution queue enhancement can help reduce the need for threads, but you still need one per connection plus extra for non-DB work. If you are playing with an implementation that uses Oracle's pool, make sure you are closing connections even on error conditions - check the pool stats to see what's going on.

[cjbj]

@atiertant I think the UV_THREADPOOL_SIZE setting code should be removed. It doesn't do anything because it set after the thread pool has already started.

@@ -22,11 +22,6 @@
 class Client_Oracledb extends Client_Oracle {
   constructor(config) {
     super(config);
-    if (this.driver) {
-      process.env.UV_THREADPOOL_SIZE = process.env.UV_THREADPOOL_SIZE || 1;
-      process.env.UV_THREADPOOL_SIZE =
-        parseInt(process.env.UV_THREADPOOL_SIZE) + this.driver.poolMax;
-    }
   }
 
   _driver() {

[aidenfoxx]

I agree that the code should be removed. I would prefer instead to limit the poolMax value based on UV_THREADPOOL_SIZE, and log a warning to the user that their threadpool size is incompatible with their requested pool size (when applicable).

Happy to submit a PR for this if @kibertoad agrees?