-
Notifications
You must be signed in to change notification settings - Fork 1.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
Memory leak with getRows() #1194
Comments
We're still going to need a runnable script that demonstrates the problem; otherwise, we are just guessing! There is a known issue with |
I will try to replicate the issue with a small example. It is a part of a custom framework and I need some time to try isolating the issue. Is there any known workaround on how to force memory release? |
Also, look through this issue: #1186 In the last message from @zymon, he provides a link to an excel document with memory usage analysis from before and after he implemented the changes I suggested. Before the changes, there was an unreasonable amount of RSS memory consumed. The changes mostly had to do with the way the JavaScript code worked with connections from the database. For example, don't use |
No there is no One http request is qual to one database call at most of the cases. |
That was just a lucky guess. :) There are lots of other ways this could happen. Could you please share some code with us? Maybe just edit any sensitive parts out? |
@dmcghan @anthony-tuininga To start the application you need to:
You will see memory usage statistics inside each response |
The first thing that jumps out at me, if I'm reading the code correctly, is that you're creating a single connection to the database and caching that in your database module. I can see how this may be the source of the trouble. You'll need to switch to using a connection pool. Update Get rid of methods like If possible, get rid of This will be more important as you move to a connection pool as you don't want to incur two round trips (one per execute) to set these values. There are other options if you can't use environment variables for some reason. Make those changes and test again. Let us know how it goes. |
@dmcghan Thank you for the response. The connection pool is already in our to-do list. I do totally agree that it would be more efficient, but I think that the issue is with Also, the current issue has only appeared after upgrading to binding approach. Before that, the same application used oracledb V3.x and we haven't had rss ram issues. |
Please try to escalate this ASAP. The current implementation is all kinds of wrong.
Ah, that is interesting. Are you calling this for a lot of classes? Now that you have a test case, you should be able to test this, no? Find out which classes you're calling the method for (hopefully there's just a few) and get the values it's returning. Then update the function so that it doesn't actually call the method - just return the values based on the string passed in. |
@dmcghan I have recreated the same case without any binds except cursor output and the leak is still there. It is almost visible, but still there. It seems that you will be right regarding leak origin being creating new database connection on each request and then closing it after request. This seems to be another issue, but the connection pool might solve this. I will upgrade connection handling and let you know about the results. |
I wouldn't expect to see the RSS issues when creating one-off connections for each request. Though I would expect performance to suck. Use a pool! :) I'm still new to your code, and I'm not a TS person, but from what I see, The only method that creates a new instance of Update your code to use a pool and let us know what happens. |
OracleDBHelper is being created on each http request, it means each time a new connection is being created. After http response is sent connection is being closed. So this still should not have any memory issues.
The pool priority went up after your comments and will be implemented soon. But there is a memory leak still somewhere with single instance creation even if pool will resolve the issues. |
I believe you, but please point out the files and relevant lines. |
Create connection: Close connection: |
I see, so App.ts line 69 is being invoked per request. The name But this does mean that you're creating a request for each http request and then invoking Please update the code and let us know how things afterward. |
It's worth testing Node.js 13, since the potential fix @anthony-tuininga pointed to only just landed in a Node.js 12 code line last night: nodejs/node#30109 |
@dmcghan @cjbj I have implemented connection pool and rss ram usage decreased around 30% but this is still way above what is expected. Is there anything I could do to help to find a solution for this issue? Also, I have tried updating node to V12.14.1 and this had 0 impact on the issue. The issue is clearly related to I am building a new docker image with node V13.x and I will let you know about the results. |
@lkiii after you've tried Node 13, you could give us a test case (not an app, e.g. give us a single javascript file) and the SQL to create the tables + data. Also run some tests and plot the results to show whether this is a leak over time. Thanks, |
Node v13 had no impact on the RAM issue. @cjbj Hello, I have provided simple test case. oracletestcase.zip |
@lkiii thanks for that work - but can you remove all the other modules to eliminate them as causes? You'll have to monitor the memory externally. |
@cjbj @dmcghan Hello, Memory usage:
|
Hi @lkiii , Thanks for the info and the simplified script! I have tried your latest script on Oracle DB 19.5, Instant Client 19.5, Node 12.13, it ran for about 10 hrs. I'm seeing some bumps in the memory but it gets stabilized. I will try some more combinations to keep trying to replicate this. I'll keep you posted. |
@j-antonio-rivera I am getting similar results if running the same script for a long time. Memory goes up and down, but the maximum value is always getting bigger by several megabytes. As I had written before it seems that there is some kind of threshold of memory after which memory usage does not increase so drastically, but the issue is that initial calls use some memory and it seems that it is not being released. I think the same thing happens with each different object so the used memory stacks if different objects are being called. I will try to replicate heavier usage with more different objects from the database. Also, you could try increasing Database version used for tests: The same behaviour is visible in both docker container with oracle linux and windows environments |
@lkiii then you are not getting similar results, because @j-antonio-rivera didn't see an increase :) |
@cjbj I am trying to tell that you do not need to look for 10 hours. The increase is most severe in several hundred first creations of As I mentioned I will try to recreate an example with more different DBOjects. |
@lkiii sounds good! And let us know your exact Oracle client & server versions. |
Server: I could also provide docker image with the environment, but the same results are happening on my local windows machine, so the issue seems not to be related to Linux/Windows node differences. |
I had built a test script to confirm my guess regarding multiple types and there is no difference with single dbobject. Trying to create a better test case with an isolated issue. |
confirmation regarding the issue in oracledb nodejs/help#2393 (comment) |
@cjbj here is the test case
To start:
The issue is that ram usage does not get back to the initial state. This example uses a small data sample. If the same this is being done with ~4000k rows and ~15 fields then ram usage after running 50 calls with the same script increases ram usage to ~500Mb This is only happening if multiple requests are being processed at the same time. |
@lkiii thanks. The other day, @j-antonio-rivera ran some tests based on your older code. There was some growth with Node.js 12, but with Node 13 memory use was flat. |
Hi @lkiii , I have been playing around with .getRows function, I have tried some different combinations and I'm seeing that the memory got stable after some time of execution, like @cjbj mentioned, I'm seeing a constant increment in memory if I use Node 12 but I verified that this got fixed by changing node version to Node 13.7. Memory used increases according to the data I'm requesting. These are my results: Object/Cursor - .getRows() (Running 1 Thread) Object/Cursor - .getRows() (Running 1 Thread) Object/Cursor - .getRows() (Running 50 Threads) I will post an update with my comparison between .getRows and .getRow findings. Regards, |
@cjbj @j-antonio-rivera The issue is only clearly visible if the have you managed to test with my docker example? |
This goes back to the first comment I made regarding |
@lkiii did you try with Node.js 13? I'd like to see what Node.js 12.15 does, when it is released. It has the (or "a"?) N-API leak fix. And follow what @dmcghan says. Also see the user doc Parallelism on a Connection. |
@lkiii Node.js 12.15 is out. Give it a whirl and let us know how it behaves. |
For now I could try building new docker images with Node.js 12.5 and 13 for you to test if it would help your team to understand what is happening. Have you managed to start it and get similar results as in comment? |
@lkiii appreciate the offer (and your continued interest). Our testing framework is not Docker based. @j-antonio-rivera was testing pretty much your code. You mentioned that your problem was caused by parallel use, which isn't apparent in the code you shared. This is why I was interested in you trying out Node.js 12.15 yourself. Of course, we've mentioned why you should remove parallel use; I can imagine cases where it will use more memory. @j-antonio-rivera is about to look at Node.js 12.15 too. |
hi all, node -v module.exports = {
connectionLimit: 100,
user: process.env.NODE_ORACLEDB_USER || "user",
password: process.env.NODE_ORACLEDB_PASSWORD || "pass",
connectString: process.env.NODE_ORACLEDB_CONNECTIONSTRING || "ip/orcl",
};
any one help?
router.post("/get_idx", function(req, res) {
console.log(
"################# START get_idx ######################"
);
const used = process.memoryUsage();
for (let key in used) { console.log(`${key} ${Math.round(used[key] / 1024 / 1024 * 100) / 100} MB`); }
try {
var jsonRequest = JSON.parse(JSON.stringify(req.body));
// console.log(jsonRequest);
var numRows = 1000;
// oracledb.fetchAsString = [oracledb.CLOB];
async function run() {
let pool;
try {
pool = await oracledb.createPool(dbConfig);
var bindvars = {
cursor: { type: oracledb.CURSOR, dir: oracledb.BIND_OUT }
};
let connection;
try {
connection = await pool.getConnection();
const result = await connection.execute(
"BEGIN :cursor := test(); END;",
bindvars, { outFormat: oracledb.OBJECT }
);
const resultSet = result.outBinds.cursor;
let rows;
do {
rows = await resultSet.getRows(numRows); // get numRows rows at a time
if (rows.length === 0) {
// no rows, or no more rows
var jsonResponse = {};
jsonResponse["ERROR"] = "0004";
jsonResponse["MESSAGE"] = "FAILED";
console.log(jsonResponse);
res.send(jsonResponse);
} else if (rows.length > 0) {
var newObj = rows.reduce((a, b) => Object.assign(a, b), {});
var jsonpush = {};
var jsonResponse = {};
var sub_ = [];
for (var i = 0; i < rows.length; i++) {
jsonpush["SYSDATE"] = rows[i].SYSDATE;
sub_.push(JSON.parse(JSON.stringify(jsonpush)));
}
jsonResponse["ERROR"] = newObj.ERROR;
jsonResponse["MESSAGE"] = newObj.MESSAGE;
jsonResponse["RESULT"] = newObj.RESULT;
jsonResponse["INFO"] = sub_;
//
// console.log(jsonResponse);
res.send(jsonResponse);
}
} while (rows.length === numRows);
// always close the ResultSet
await resultSet.close();
} catch (err) {
console.error(err);
var jsonResponse = {};
jsonResponse["ERROR"] = "0003";
jsonResponse["MESSAGE"] = "FAILED";
console.log(jsonResponse);
res.send(jsonResponse);
} finally {
if (connection) {
try {
await connection.close(); // Put the connection back in the pool
} catch (err) {
console.error(err);
var jsonResponse = {};
jsonResponse["ERROR"] = "0003";
jsonResponse["MESSAGE"] = "FAILED";
res.send(jsonResponse); //
}
}
}
} catch (err) {
console.error(err);
var jsonResponse = {};
jsonResponse["ERROR"] = "0003";
jsonResponse["MESSAGE"] = "FAILED";
console.log(jsonResponse);
res.send(jsonResponse);
} finally {
await pool.close();
}
}
run();
} catch (e) {
console.log("Exception in get_idx: " + e);
jsonResponse["ERROR"] = "-1";
jsonResponse["MESSAGE"] = "FAILED";
res.send(jsonResponse);
} finally {
res.status(200);
console.log("################# get_idx ######################");
}
}); |
|
And do not create the pool on each request! Create one pool as part of your application initialization outside the router. (It would be helpful if you can tell me why you did this, so I can improve documentation and examples). |
Is If your table contains an exact multiple of 1000 rows, you will be sending FAILED. At some stage we reworked the
Also you may find the (singular) |
can u example create pool outsite the router? im have more router.post(...) im have 2000 row in database |
The closest example I have is https://github.com/oracle/node-oracledb/blob/master/examples/webappawait.js
You mean increase numRows? I wouldn't do that. Just fix your code regarding your end-of-fetch logic. Do the other things suggested, particularly upgrading, and let us know the outcome. |
Hi, I have tried recent versions of Node JS 12.18.x, & 14.4.0, also I've used for this execution Oracle Instant Client 19.6 pointing to Oracle DB 19.6, using a single and multiple threads executions. The behavior of the results are similar to what I have reported on my comment on Jan 31, this issue gets improved by the fix of N-API issue 28988 These are my results: Object/Cursor - .getRows() (Running 1 Thread) Object/Cursor - .getRows() (Running 50 Threads) Object/Cursor - .getRows() (Running 1 Thread) Object/Cursor - .getRows() (Running 50 Threads) I will run a couple of more executions with more iterations since in my older testing I didn't see the minor bumps I'm seeing now in the single threaded executions, I will post and update once I have completed those executions. |
After discussion, I'm going to close this issue. The big memory leak does seems to have been the Node.js N-API bug. The fix has trickled into supported Node.js versions. Yay! We'll continue to monitor and test and resolve issues as we find them. |
|
@karkan20 is it a real leak or did the GC clean up eventually? I don't recall seeing the CREATE TABLE statement. Can you share that and a current, complete & runnable testcase? |
@cjbj it looks like a real leak where the object perisits and never gets cleaned up, the memory consumption graph keeps going up. On restarting the nodejs app released the memory. Not sure about the create table statement. I am getting resultset and then getting rows in batch of getrows(1024) are fetched and then transform it my requirement, then get another batch and so on. Even if change the batch size to like 50 it still consumes and never gets released unless app restarts. |
@karkan20 what are the column types? |
String, date, number |
@karkan20 As indicated by @cjbj, request your help to please share a runnable test case. This will help us get to the root cause of the problem. |
Also, If there are no rows going to be fetched, we should always close the resultSet. |
Out of memory. RAM usage.
This output is visible after ~20 http rest api calls to the server and each request accesses database via oracledb.
I am closing all the connections and triggering garbage collector manually.
oracledb is used to call stored procedures and functions with object binds. I am using
connection.getDbObjectClass()
for the binds and I suspect this could be the cause of the memory issues. It seems that all the issues have started after upgrading to this approach.12c
Here is the backstory, how I have come to a conclusion that this is oracledb related issue nodejs/help#2393
The text was updated successfully, but these errors were encountered: