ORACLEDB: Updated handling of connection errors for disposal

[cemremengu]

Solves #2514 and #2603. I tested it on my-machine 😄 I am thinking that simulating this will be tricky and probably take about 15-20 minutes for oracle (or maybe I am exaggerating)

In addition to updating connection error list, we also need to update oracledb to check for connection errors.

Disclaimer: I am not good with vanillajs (prototypes etc.) so it would be great if someone reviews the syntax/approach.

[elhigu]

To me code looks pretty good, I would need to checkout the connection.__knex__disposed setting if that is the thing that should be done there (probably it is correct though). @atiertant how does tihs looks to you?

[cemremengu]

One thing I want to add is that the connections are disposed on the knex side but the database is never notified about this due to communication error. The inactive sessions/connections stay on the database side. Not sure if the database eventually kills them though depending on the settings. This flow can be represented as:

Client:
1) grabbed a connection 
2) hit an exception 
3) propagated over and out of the scope of the connection handle from (a) 
4) so it can never give it back - it lost the track of connection 

Is there any way you know that can help this issue or any generally related experience?

[atiertant]

doesn't client.isConnectionError reference the same fonction ?

[atiertant]

@cemremengu oracle database has a dead connection detection mechanism but we should call connection.close before set the connection.__knex__disposed.
@elhigu looks great, but didn't tested it

[cemremengu]

@atiertant calling connection.close seems to throw an error though so I am not sure if it achieves anything

 ERROR 8672 nodejs.unhandledRejectionError: NJS-003: invalid connection

[atiertant]

@cemremengu see node-oracledb examples
https://github.com/oracle/node-oracledb/blob/master/examples/select1.js#L73
should ask @cjbj on a node-oracledb issue which errors needs to close the connection and which other need only to remove the connection object from pool ...
can't find this kind of mechanism in the node-oracledb driver pool, does it has the same bug ?

[cemremengu]

Created #913

[cemremengu]

Added connection.close() to conform what @cjbj stated

Closing the connection is the way to remove it from the connection pool, which is important so the pool slot can be reused by a new valid connection. You should always call conn.close(). What you may want to do is ignore errors on the close.

[cjbj]

See https://github.com/oracle/odpi/blob/master/src/dpiError.c#L65 for a bigger error list used by the node-oracledb connection pool. Node-oracledb also uses an OCI call at https://github.com/oracle/odpi/blob/29968fc2085941c40fce8f56a42c9920305d2e29/src/dpiConn.c#L204 which does most of the same checks anyway.

[cjbj]

NJS-024 is duplicated a few lines above. And arguably shouldn't be on the list.

[cjbj]

The issue heading mentions oracledb but the files changed are for dialect/oracle, not dialect/oracledb. Is this what you intended?

Also, my comments about connection pooling are for the node-oracledb connection pool, which isn't used in dialects/oracledb

[elhigu]

@cjbj looks like oracledb is actually inherited from oracle, so that might explain the changes in the oracle directory.

[kibertoad]

@cjbj Does that mean that currently used list of errors that should close connections is invalid in case of not using oracledb connection pool?

[cjbj]

@kibertoad the ORA errors that indicate a bad connection are pretty much going to be the same for pooled & non-pooled connections. You could check for the same numbers that ODPI-C uses, see my earlier link.

[kibertoad]

@cemremengu Could you please provide me write permissions to your repo so that I could fix the PR accordingly to cjbj's comments without forking and resubmitting the pr?

[kibertoad]

@cjbj Thanks!

[cemremengu]

@kibertoad Sorry, snowed under work lately. Updated the error list accordingly

Would be great if you test and give feedback on this patch!

[kibertoad]

@cemremengu looks like there are random failures, can you rerun tests by closing and opening pr?

[cemremengu]

@kibertoad done. Also given you permission in case you want to tweak stuff

[kibertoad]

awesome, thank you! also tests passed, yay :)

[elhigu]

I think this could be tested by mocking/monkeypatching client.isConnectionError method and returning the original implementation after tests.

[kibertoad]

@elhigu What should such test assert?

[elhigu]

@kibertoad that connection state is set to disposed in all of the changed code paths (test should fail before this change for some paths).

[alanchristensen]

What's the status of this PR?

[kibertoad]

@alanchristensen I could resolve the merge conflict, but would really appreciate it if someone else wrote the test :)

[pablopen]

Hi, any updates on this?
Looks like it could solve the issues when oracle connections are consuming one pool's connection and failing each request that fall in that connection.

[kibertoad]

@pablopen Any chance you would volunteer to write at least some test for it?

[pablopen]

@kibertoad I could try. I'll check elhigu suggestion of mocking the errors.

[kibertoad]

@pablopen You are my hero :)

[elhigu]

I would still have preferred having some tests for this, now this is waiting for regression to happen if anyone ever touches that code.

[kibertoad]

@elhigu In ideal world, yes. However, year passed, and noone did, and I lost all hope to ever get to it myself; and this change is both straightforward enough and useful (as evidenced by already reported problems solved by it).

[elhigu]

In this particular case I considered also that benefits were bigger than risks of it causing problems later on.