-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ORACLEDB: Updated handling of connection errors for disposal #2608
Conversation
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? |
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:
Is there any way you know that can help this issue or any generally related experience? |
src/dialects/oracledb/index.js
Outdated
@@ -135,13 +135,19 @@ Client_Oracledb.prototype.acquireRawConnection = function() { | |||
if (options.resultSet) { | |||
connection.execute(sql, bindParams || [], options, function(err, result) { | |||
if (err) { | |||
if (Client_Oracle.prototype.isConnectionError(err)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't client.isConnectionError reference the same fonction ?
@cemremengu oracle database has a dead connection detection mechanism but we should call connection.close before set the connection.__knex__disposed. |
@atiertant calling
|
@cemremengu see node-oracledb examples |
Created #913 |
Added
|
src/dialects/oracle/index.js
Outdated
|
||
// If the error is any of these, we'll assume we need to | ||
// mark the connection as failed | ||
isConnectionError(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/dialects/oracle/index.js
Outdated
'NJS-040', | ||
'NJS-024', | ||
'NJS-003', | ||
'NJS-024', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NJS-024 is duplicated a few lines above. And arguably shouldn't be on the list.
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 |
@cjbj looks like oracledb is actually inherited from oracle, so that might explain the changes in the |
@cjbj Does that mean that currently used list of errors that should close connections is invalid in case of not using oracledb connection pool? |
@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. |
@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? |
@cjbj Thanks! |
@kibertoad Sorry, snowed under work lately. Updated the error list accordingly Would be great if you test and give feedback on this patch! |
@cemremengu looks like there are random failures, can you rerun tests by closing and opening pr? |
@kibertoad done. Also given you permission in case you want to tweak stuff |
awesome, thank you! also tests passed, yay :) |
I think this could be tested by mocking/monkeypatching client.isConnectionError method and returning the original implementation after tests. |
@elhigu What should such test assert? |
@kibertoad that connection state is set to disposed in all of the changed code paths (test should fail before this change for some paths). |
What's the status of this PR? |
@alanchristensen I could resolve the merge conflict, but would really appreciate it if someone else wrote the test :) |
Hi, any updates on this? |
@pablopen Any chance you would volunteer to write at least some test for it? |
…e-connection-error
@kibertoad I could try. I'll check elhigu suggestion of mocking the errors. |
@pablopen You are my hero :) |
…e-connection-error
…e-connection-error # Conflicts: # src/dialects/oracle/index.js
…to oracle-connection-error # Conflicts: # package.json
I would still have preferred having some tests for this, now this is waiting for regression to happen if anyone ever touches that code. |
@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). |
In this particular case I considered also that benefits were bigger than risks of it causing problems later on. |
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.