Node-oracledb: Readme-Example does not close its connection

[stealthybox]

1. What error(s) you are seeing?

After basing application code off of the example code in the README, our team experienced leaked connections to our database.

2. What caused the problem?

The example code does not demonstrate how to explicitly close connections.
When running queries intermittently in a loop, large numbers of connections can build up over time.

[cjbj]

We had discussions about this and so far I've won - but that may change :) The README example is a simple 'this is what it looks like' demo. The https://github.com/oracle/node-oracledb/tree/master/examples are the demos to follow for practical examples.

[stealthybox]

@cjbj oops, I just submitted a PR I was staging
-- sorry I didn't see your comment
(I was signing my OCA)

Could we throw in a note that points people in the right direction then?
My use case was very simple so I grabbed that code snippet assuming it would be compliant with the practices in the examples directory.

Our devs aren't familiar with this driver yet, so leaky code made it into our build pipeline.
Thanks for the quick response!

[sagiegurari]

Have to agree with @cjbj
The examples also won't show how you get the connection in the first place as well.
If you go to that direction there are just too many permutations and the examples will be a real mess that no one will understand.
For example others will complain why after every execute they need to release the connection and can't reuse it, and why? Because the example had a release after execute.
Developers need to read the basics before coding.

[bchr02]

@cjbj @sagiegurari I have to disagree. The section is titled "A simple query example with callbacks" and not "An incomplete example of what the API looks like". I think the current title is fine, but it should include something as basic and necessary as closing the connection. Even a basic query needs to close its connection, right? Leaving it is clearly misleading, otherwise we would't have this issue. Anyways that's my thoughts. 😃

@sagiegurari Where you write:

The examples also won't show how you get the connection in the first place as well.

I think you are incorrect with that statement because the example does show this.

If you go to that direction there are just too many permutations and the examples will be a real mess that no one will understand.

There is currently only one example on the main README's Example section which is the subject of this issue. So, I don't think there is an issue of "too many permutations". Even if additional example are added they will have a sub section heading.

others will complain why after every execute they need to release the connection and can't reuse it, and why? Because the example had a release after execute.

By this logic we shouldn't have any example because people will think whatever they want to think.

Developers need to read the basics before coding.

I agree, but if someone is just dabbling at using node with oracledb and they see a section titled "A simple query example..." one could argue that this is the basics.

Again, as I said earlier, these are just my thoughts and logic on the issue.

[sagiegurari]

@bchr02 actually i'm not looking at the readme example which is not even an example as I see it, just a small taste of the api which causes more confusion than help.
I was looking at the api doc where you have actual examples: https://github.com/oracle/node-oracledb/blob/master/doc/api.md#fetchingrows (and so on...)
there you can see, no get connection and no release connection.
you can't learn how to use this library from just the readme without the api.md

I'll say even more, I would drop the example from the readme completely.
I know @cjbj complains about people using connection from oracledb and not via pool and that kind of super simple readmes are part of the problem.
instead the readme should point to main sections/examples from the api doc.

[bchr02]

I'll say even more, I would drop the example from the readme completely.

Perhaps this would solve the issue, but it would arguably make it more difficult for people to feel comfortable with the API from the very beginning.

[sagiegurari]

@bchr02 don't ignore the second part of what i wrote:
_instead the readme should point to main sections/examples from the api doc._

[stealthybox]

Thanks for the discussion everyone.

For my use case, the example query was almost exactly what I needed.
I don't need a connection pool;
My app talks to an oracle db for a few milliseconds on a set interval.

I really just needed a pointer somewhere that showed me I needed to close the connection after querying.
IMO, #530 is a trivial change, and it makes the example code a safe starting point for new users.

[cjbj]

We had some (abbreviated) discussions about this yesterday.

How about we remove the sample from the README but add the close() call to the (same) sample in api.md?

[stealthybox]

Thanks @cjbj !
Gonna be honest,
I had no idea the example from the README was duplicated in the Introduction section of the API doc.

The API doc is pretty long.
When I was looking for a close method, I Ctrl+F'd what I was looking for and went back to solving other puzzles.
It wouldn't surprise me if a lot of users consume the API like that.

I'm fine with whatever the change looks like as long as the goal is to quickly point a new user to safe example code.

I could refactor #530 to update api.md like you mentioned, and update the README to link to the example in api.md in addition to the examples/ dir.
Does that sound good?

IMO, a code sample in the README is common in the Node community,
but whatever the team thinks is best is what should go.
Appreciate the follow up!

[cjbj]

I'll think about it a bit.

I really do appreciate your PR but the OCA can take a bit of time (they batch them up, then submitters make mistakes etc) so don't worry unless you feel the need (let me know whether to wait for it)

[stealthybox]

For sure :)
It's cool to see a thriving community around OracleDB and Node.

I'm unsure of what you want me to confirm you should wait for.
I can definitely relate. Enterprise paperwork takes time to process.
If that's what you're referring to, my OCA was submitted 10 days ago and is hopefully in the pipeline :+1:

[cjbj]

I just saw the OCA approved!

[stealthybox]

Nice!
So it looks like the solution is to:

• update the api.md example
• remove the example from README
• link to the short example in api.md and examples/ from the README

@cjbj: Have you given thought as to leaving the example in the README ?
I'll update the PR when we clarify :+1:

[cjbj]

@stealthybox my thoughts are to

• remove the example from the README
• leave the existing list to examples/ from the README
• update the simple example in api.md

However what's the best way to direct people to a good, canonical example?

Also we should have an example using the pool cache

[stealthybox]

However what's the best way to direct people to a good, canonical example?

2 ideas:

• either, link to the api.md#example from README before the link to examples/
• or, add the simple query to examples/ and add an link to that from README

Good thinking on the connection pool example.
What's a good end-to-end use case?

We should probably :

• createPool with some options
• pool.getConnection -> selects... (In a long running loop / server?)
• pool.getConnection -> inserts...
• monitor pool properties
• ensure connections are closed
• pool.close

Maybe we could show an example with an http or express server?

[cjbj]

@stealthybox What about the link from README naming which example the reader should start with (e.g select1.js)?

The webapp*.js examples already use http. I was just playing with a variant the uses async but it may not be so useful to push - it just cleans up the error handling at the expense of other cruft.

Other examples could do with a cleanup (PR's welcome).

[cjbj]

Closing this - check out the change in the 1.12.0-dev release on GitHub. Thanks everyone for helping making node-oracledb better.

[stealthybox]

Nice Changes!
Thanks :+1: