sagiegurari / simple-oracledb

Extend capabilities of oracledb with simplified API for quicker development.
Apache License 2.0
36 stars 4 forks source link

pool.run - onActionDone callback function abnomal #4

Closed sukjaelee closed 8 years ago

sukjaelee commented 8 years ago

Problem Description

pool.run(function (connection, callback) { //run some query and the output will be available in the 'run' callback connection.query('SELECT department_id, department_name FROM departments WHERE manager_id < :id', [110], callback); }, function onActionDone(error, result) { //do something with the result/error });

pool.runs OK at first call. But after first call, onActionDone callback function is not called or reponse too late.

I use "oracledb": "1.8.0"

Error Stack/Info (if any)

sagiegurari commented 8 years ago

Can you please copy paste the code that fails?

sukjaelee commented 8 years ago
'use strict';
var oracledb = require ('oracledb');
var dbconfig = require ('./dbconfig.js');
var SimpleOracleDB = require ('simple-oracledb');
SimpleOracleDB.extend (oracledb);
var myPool;
exports.register = function (server, options, next) {
    server.method ({
            name: 'database.getRecent',
            method: function (callback) {
                //pool.getConnection : OK
 /*
                myPool.getConnection (function onConnection(poolError, connection) {
                    if (poolError) {
                        connection.release ();
                        console.log ('poolError=' + poolError);
                        throw poolError;
                    }
                    connection.query ('SELECT code, lat, lng, timestamp FROM PINGS order by timestamp desc',
                        {},
                        function onResults(error, results) {
                            if (error) {
                                console.log ('error=' + error);
                                throw error;
                            } else {
                                connection.release ();
                                callback (null, results);
                            }
                        });
                });
*/
                //pool.run : fail to release connection
                myPool.run (function (connection, callback) {
                        connection.query ('SELECT code, lat, lng, timestamp FROM PINGS order by timestamp desc', {}, callback);
                    }, function onActionDone(error, results) {
                        if (error) {
                            console.log ('error=' + error);
                            throw error;
                        } else {
                            callback (null, results);
                        }
                    }
                );
            }
        }
    );
    oracledb.createPool ({
            poolMax: 10,
            poolMin: 2,
            poolIncrement: 4,
            poolTimeout: 4,
            retryCount: 5, //The max amount of retries to get a connection from the pool in case of any error (default to 10 if not provided)
            retryInterval: 500, //The interval in millies between get connection retry attempts (defaults to 250 millies if not provided)
            runValidationSQL: true, //True to ensure the connection returned is valid by running a test validation SQL (defaults to true)
            validationSQL: 'SELECT 1 FROM DUAL', //The test SQL to invoke before returning a connection to validate the connection is open (defaults to 'SELECT 1 FROM DUAL')
            user: dbconfig.user,
            password: dbconfig.password,
            connectString: dbconfig.connectString
        },
        function (err, pool) {
            console.log ("pool created");
            SimpleOracleDB.extend (pool);
            myPool = pool;
        });
    next ();
}
;
exports.register.attributes = {
    pkg: require ('./package')
};
sagiegurari commented 8 years ago

you have many issues in your code, mostly have to do with not calling the callback. for example (and you have several of these):

if (error) {
   console.log ('error=' + error);
   throw error;
 } 

instead of:

console.log(.....);
callback(error); //this is missing

you should always always call the callback.

sagiegurari commented 8 years ago

if you want more comments

function onActionDone(error, results) {
  if (error) {
    console.log ('error=' + error);
  }

  callback(error, results);
}

no need for the else block at all.

sukjaelee commented 8 years ago

I modified the program as your recommends, but it did not help. I mailed to you. If you have time, would you please review the source?

sagiegurari commented 8 years ago

I didn't get any mail with your code.

sagiegurari commented 8 years ago

anyway, didn't get your code, but I modified the code that you posted here and fixed all issues which I saw, that included:

'use strict';

var oracledb = require('oracledb');
var dbconfig = require('./dbconfig.js');

var SimpleOracleDB = require('simple-oracledb');
SimpleOracleDB.extend(oracledb);

var myPool;

exports.register = function (server, options, next) {
    server.method({
        name: 'database.getRecent',
        method: function (requestCallback) {
            myPool.run(function (connection, callback) {
                    connection.query('SELECT code, lat, lng, timestamp FROM PINGS order by timestamp desc', {}, callback);
                }, function onActionDone(error, results) {
                    if (error) {
                        console.log('error=', error);
                    }

                    requestCallback(error, results);
                }
            );
        }
    });

    oracledb.createPool({
        poolMax: 10,
        poolMin: 2,
        poolIncrement: 4,
        poolTimeout: 4,
        retryCount: 5, //The max amount of retries to get a connection from the pool in case of any error (default to 10 if not provided)
        retryInterval: 500, //The interval in millies between get connection retry attempts (defaults to 250 millies if not provided)
        runValidationSQL: true, //True to ensure the connection returned is valid by running a test validation SQL (defaults to true)
        validationSQL: 'SELECT 1 FROM DUAL', //The test SQL to invoke before returning a connection to validate the connection is open (defaults to 'SELECT 1 FROM DUAL')

        user: dbconfig.user,
        password: dbconfig.password,
        connectString: dbconfig.connectString
    }, function (err, pool) {
        console.log("pool created");
        myPool = pool;

        next();
    });
};

exports.register.attributes = {
    pkg: require('./package')
};
sukjaelee commented 8 years ago

Hi,

This is my source.

hapi-7.4.3-simple-oracledb.tar.gz

sagiegurari commented 8 years ago

Hope this fixes all issues. your code is really filled with bugs. mainly with you calling callback but than the function continues to do stuff. like in case of connection error, you call the callback, but your function than uses that connection to do a query. so i hope i fixed all these, but trust me the issue is not with pool.run which I use today in production but with your misunderstanding of how to use callbacks.

'use strict';

var oracledb = require('oracledb');
var dbconfig = require('./dbconfig.js');

var SimpleOracleDB = require('simple-oracledb');
SimpleOracleDB.extend(oracledb);

var myPool;

exports.register = function (server, options, next) {

    server.method({
        name: 'database.getRecent',
        method: function (requestCallback) {
            myPool.run(function (connection, callback) {
                        connection.query('SELECT code, lat, lng, timestamp FROM PINGS order by timestamp desc', {}, callback);
                    }, function onActionDone(error, results) {
                        if (error) {
                            console.log('error=', error);
                        }

                        requestCallback(error, results);

                    }
            );
        }
    });

    server.method({
        name: 'database.getFlight',
        method: function (code, callback) {

            myPool.getConnection(function onConnection(error, connection) {

                if (error) {
                    connection.release();
                    console.log('error=' + error);
                    callback(error, null);
                } else {
                    connection.query('SELECT code, lat, lng, timestamp FROM PINGS where code = :code order by timestamp desc',
                            {code: code}, //binds
                            function onResults(error, results) {
                                connection.release();

                                if (error) {
                                    console.log('error=' + error);
                                    callback(error, null);
                                } else {
                                    callback(null, results);
                                }
                            });
                }
            });

        }
    });

    server.method({
        name: 'database.addPing',
        method: function (ping, callback) {

            myPool.getConnection(function onConnection(error, connection) {

                if (error) {
                    connection.release();
                    console.log('error=' + error);
                    callback(error, null);
                } else {
                    connection.insert('INSERT INTO pings(code, lat, lng, timestamp) VALUES (:code, :lat, :lng, :timestamp)',
                            {
                                code: ping.code,
                                lat: ping.lat,
                                lng: ping.lng,
                                timestamp: ping.timestamp
                            },
                            {autoCommit: true},
                            function onResults(error, results) {
                                connection.release();
                                if (error) {
                                    console.log('error=' + error);
                                    callback(error, null);
                                } else {

                                    console.log("Rows inserted: " + results.rowsAffected);  // 1

                                    callback();
                                }
                            });
                }
            });
        }
    });

    server.method({
        name: 'database.addPings',
        method: function (pings, callback) {

            var bindParamsArray = [];

            //should use bind parameters only, else errors are occured
            pings.forEach(function (ping, index, array) {
                var ping2 = {};

                ping2.code = ping.code;
                ping2.lat = ping.lat;
                ping2.lng = ping.lng;
                // ping2.timestamp = ping.timestamp;

                bindParamsArray.push(ping2);
            });

            myPool.getConnection(function onConnection(error, connection) {

                if (error) {
                    connection.release();
                    console.log('error=' + error);
                    callback(error, null);
                } else {
                    connection.batchInsert('INSERT INTO pings(code, lat, lng) VALUES (:code, :lat, :lng)',
                            bindParamsArray,
                            {autoCommit: true},
                            function onResults(error, results) {
                                connection.release();

                                if (error) {
                                    console.log('error=' + error);
                                    callback(error, null);
                                } else {
                                    results.forEach(function (result, index, array) {
                                        console.log("index=%d,  result.rowsAffected=%d", index, result.rowsAffected);  // 1
                                    });

                                    callback();
                                }
                            });
                }
            });

        }
    });

    server.method({
        name: 'database.transaction',
        method: function (pings, callback) {

            var insertPing = {};
            var deletePing = {};
            var updatePing = {};

            pings.forEach(function (ping, index, array) {

                if (ping.transType === "I") {
                    insertPing.code = ping.code;
                    insertPing.lat = ping.lat;
                    insertPing.lng = ping.lng;
                } else if (ping.transType === "U") {
                    updatePing.code = ping.code;
                    updatePing.lat = ping.lat;
                    updatePing.lng = ping.lng;
                } else if (ping.transType === "D") {
                    deletePing.code = ping.code;
                }
            });

            myPool.getConnection(function onConnection(error, connection) {

                if (error) {
                    connection.release();
                    console.log('error=' + error);
                    callback(error, null);
                } else {
                    //run all actions in sequence
                    connection.transaction([
                        function firstAction(callback) {

                            connection.insert('INSERT INTO pings(code, lat, lng, timestamp) VALUES (:code, :lat, :lng, :timestamp)',
                                    {
                                        code: insertPing.code,
                                        lat: insertPing.lat,
                                        lng: insertPing.lng,
                                        timestamp: insertPing.timestamp
                                    },
                                    function onResults(error, results) {
                                        if (error) {
                                            console.log('error=' + error);
                                            callback(error, null);
                                        } else {
                                            console.log("Rows inserted: " + results.rowsAffected);  // 1
                                            callback();
                                        }

                                    });

                        },
                        function secondAction(callback) {
                            connection.insert('update  pings set lat = :lat, lng = :lng, timestamp = :timestamp where code = :code',
                                    {
                                        code: updatePing.code,
                                        lat: updatePing.lat,
                                        lng: updatePing.lng,
                                        timestamp: updatePing.timestamp
                                    },
                                    function onResults(error, results) {
                                        if (error) {
                                            console.log('error=' + error);
                                            callback(error, null);
                                        } else {
                                            console.log("Rows updated: " + results.rowsAffected);  // 1
                                            callback();
                                        }
                                    });
                        },
                        function thirdAction(callback) {
                            connection.insert('delete from pings where code = :code',
                                    {
                                        code: deletePing.code
                                    },
                                    function onResults(error, results) {
                                        if (error) {
                                            console.log('error=' + error);
                                            callback(error, null);
                                        } else {
                                            console.log("Rows deleted: " + results.rowsAffected);  // 1
                                            callback();
                                        }
                                    });
                        }
                    ], {
                        sequence: true
                    }, function onTransactionResults(error, results) {
                        console.log("trans=%d", results.length);

                        connection.release();
                        callback();
                    });
                }
            });

        }
    });

    server.method({
        name: 'database.terminatePool',
        method: function (callback) {
            /*      database.terminatePool()
             .then(function () {
             console.log('node-oracledb connection pool terminated');
             })
             .catch(function (err) {
             console.error('Error occurred while terminating node-oracledb connection pool', err);
             });*/
        }
    });

    oracledb.createPool({
                poolMax: 30,
                poolMin: 2,
                poolIncrement: 1,
                poolTimeout: 4,
                retryCount: 5, //The max amount of retries to get a connection from the pool in case of any error (default to 10 if not provided)
                retryInterval: 500, //The interval in millies between get connection retry attempts (defaults to 250 millies if not provided)
                runValidationSQL: true, //True to ensure the connection returned is valid by running a test validation SQL (defaults to true)
                validationSQL: 'SELECT 1 FROM DUAL', //The test SQL to invoke before returning a connection to validate the connection is open (defaults to 'SELECT 1 FROM DUAL')

                user: dbconfig.user,
                password: dbconfig.password,
                connectString: dbconfig.connectString
            },
            function (err, pool) {

                if (err) {
                    console.log('pool error=' + err);
                } else {
                    console.log("pool created");
                }
                myPool = pool;

                next(err);
            });
};

exports.register.attributes = {
    pkg: require('./package')
};
sukjaelee commented 8 years ago

I found the problem.

I reviewed the your Pool.prototype.run function. At there

        if (releaseOptions.force === undefined) {
            releaseOptions.force = true;
        }

and you commented

It occured to me. So I give the releaseOptions like this

                myPool.run(function (connection, callback) {
                        connection.query('SELECT code, lat, lng, timestamp FROM PINGS order by timestamp desc', {}, callback);
                    },
                    {
                        releaseOptions: {force: false}
                    },
                    function onActionDone(error, results) {
                        if (error) {
                            console.log('error=', error);
                        }
                        requestCallback(error, results);
                    }
                );

And the problem has gone. Is there any reason releaseOptions.force' default option to false?

        if (releaseOptions.force === undefined) {
            releaseOptions.force = false;
        }
sagiegurari commented 8 years ago

wait, first of all, did you take the code changes i gave you?

second, the code is not default false in the pool.run, it is default true: https://github.com/sagiegurari/simple-oracledb/blob/fa555092c7d8614aeb156295a78112e0317fa696/lib/pool.js#L201-L205

if (releaseOptions.force === undefined) {
            releaseOptions.force = true;
        }

third open the debug to see for any issues: https://github.com/sagiegurari/simple-oracledb#debug

I'm using pool.run in production in an app we wrote. trust me it works very good under high load. But you must understand callbacks and how the code flows in js, specifically in node.js with io threads compared to js single thread.

sukjaelee commented 8 years ago

I had tested the code you gave me. The same problem occured. Just when I add {releaseOptions: {force: false}}, it run well. I changed the source to pool.run operations. Let me attach the my source. It has just one table. When you read the README.md, you can fiind the table create query, and initial load data script. If you run the program you will find the my problem. hapi-7.4.3-simple-oracledb.tar.gz

I actually java programmer, I just studied node.js. Your library great helpful to me.

sagiegurari commented 8 years ago

Any output when you put the debug flag I gave a link for? Any internal release error would be printed.

I will review the updated code you attached latter today/tomorrow

sagiegurari commented 8 years ago

lines 148 until 179 might (probably not but still) have an issue since you have callback declared in pool.run(function (connection, callback) { and also in every internal action there function firstAction(callback) { function secondAction(callback) { function thirdAction(callback) { Still, wouldn't say that is the issue, but it is an issue. also when you say fail, what does that mean? error stack?

sukjaelee commented 8 years ago

This is sunday. I do not have a oracle in my notebook. So I again tested with oracle docker image in https://hub.docker.com/r/wnameless/oracle-xe-11g/ with the same source. All is fine. releaseOptions: {force: false} does not matter. But in my company's oracle test environment I should set releaseOptions: {force: false}. Tomorrow I will test again in company's oracle test environment and new oracle installation environment. thanks many.

sukjaelee commented 8 years ago

I tested with same source in three oracle db environments. the result is

  1. oracle docker image above - OK
  2. new installation of Oracle Database 12c Release 1 Enterprise Edition - OK
  3. the existing oracle test db - I should set releaseOptions: {force: false}

So we are going to drop the existing oracle test db and start pilot project with the new installation of Oracle Database 12c Release 1 Enterprise Edition.

You can close the issue. I've run a lot for your help. thank you a lot.

sagiegurari commented 8 years ago

lets keep the issue open until you test with a new DB. once you finish testing tell me and you can close the issue.