Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ORACLEDB: Updated handling of connection errors for disposal #2608

Merged
merged 12 commits into from Jun 19, 2019

Conversation

Projects
None yet
7 participants
@cemremengu
Copy link
Contributor

commented May 15, 2018

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.

@cemremengu cemremengu changed the title Updated handling of connection errors to dispose connections correctly ORACLEDB: Updated handling of connection errors to dispose connections correctly May 15, 2018

@cemremengu cemremengu changed the title ORACLEDB: Updated handling of connection errors to dispose connections correctly ORACLEDB: Updated handling of connection errors for disposal May 15, 2018

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

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?

@@ -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)) {

This comment has been minimized.

Copy link
@atiertant

atiertant May 16, 2018

Contributor

doesn't client.isConnectionError reference the same fonction ?

@atiertant

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

Created #913

@cemremengu

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2018

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.


// If the error is any of these, we'll assume we need to
// mark the connection as failed
isConnectionError(err) {

This comment has been minimized.

Copy link
@cjbj

cjbj May 21, 2018

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.

'NJS-040',
'NJS-024',
'NJS-003',
'NJS-024',

This comment has been minimized.

Copy link
@cjbj

cjbj May 21, 2018

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

@cjbj

This comment has been minimized.

Copy link

commented May 21, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2018

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2018

@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

This comment has been minimized.

Copy link

commented Jun 5, 2018

@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

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2018

@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

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2018

@cjbj Thanks!

@cemremengu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

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

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2018

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

@cemremengu cemremengu closed this Jun 6, 2018

@cemremengu cemremengu reopened this Jun 6, 2018

@cemremengu

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2018

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

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2018

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2018

@elhigu What should such test assert?

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2018

@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

This comment has been minimized.

Copy link

commented Aug 22, 2018

What's the status of this PR?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2018

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

@pablopen

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

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

@pablopen

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2019

@pablopen You are my hero :)

Igor Savin and others added some commits Apr 12, 2019

Igor Savin
Igor Savin
Merge branch 'master' of https://github.com/tgriesser/knex into oracl鈥
鈥-connection-error

# Conflicts:
#	src/dialects/oracle/index.js
Merge remote-tracking branch 'cemrenmengu/oracle-connection-error' in鈥
鈥o oracle-connection-error

# Conflicts:
#	package.json

@kibertoad kibertoad merged commit 6e5e296 into tgriesser:master Jun 19, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 88.079%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kibertoad kibertoad deleted the cemremengu:oracle-connection-error branch Jun 19, 2019

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.