oracle / node-oracledb

Oracle Database driver for Node.js maintained by Oracle Corp.
http://oracle.github.io/node-oracledb/
Other
2.25k stars 1.07k forks source link

Capture stack trace for error #1467

Closed sla100 closed 2 years ago

sla100 commented 2 years ago

Solution for https://github.com/oracle/node-oracledb/issues/465

Signed-off-by: Sławomir Osoba <sla100@poczta.fm>
cjbj commented 2 years ago

@sla100 when I did a (very) quick test yesterday, moving the new Error() call after catch still showed the failing point of code when there was a connection error, even if other layers weren't shown.

      return func.apply(this, arguments).catch(function(e){
        const context = new Error('#');
        e.stack += context.stack.replace(stacktraceRe, '');
        throw e;
      });

gave:

cjones@mac:~/n$ node t.js
Error: ORA-01017: invalid username/password; logon denied
    at /Users/cjones/n/node-oracledb/lib/util.js:182:25
    at async run (/Users/cjones/n/t.js:25:18) {
  errorNum: 1017,
  offset: 0
}

Which correctly identified line 25 of t.js as having my getConnection() call.

I only did the one test....

But if this is valid, would it be just as good a solution for identifying where the error was? One thing that concerned me when I looked at https://github.com/oracle/node-oracledb/issues/465 was the overhead of calling new Error() for each node-oracledb API call.

I'd be tempted to strip out the util.js from the stack

sla100 commented 2 years ago

It seems to me that since Node 16 which using V8-7 the situation has improved: https://v8.dev/blog/v8-release-73#async-stack-traces https://nodejs.org/en/download/releases/

When we limit ourselves to async / await the change becomes very light. I changed my code.

cjbj commented 2 years ago

@sla100 I'd be fine with support just for async/await.

From another quick test, what if you name the function so its stack can be omitted?:

      return func.apply(this, arguments).catch(function st(e) {
        Error.captureStackTrace(e, st);
        throw e;
      });

With this, a connection error is like:

Error: ORA-01017: invalid username/password; logon denied
    at async run (/Users/cjones/n/t.js:25:18) {
  errorNum: 1017,
  offset: 0
sla100 commented 2 years ago

Sure. It will be clearer.

cjbj commented 2 years ago

@sla100 any news on your OCA? Searching for your names in the system isn't showing me anything.

sla100 commented 2 years ago

Nope. I send email to opensource_ww_grp@oracle.com and no answer.

abrar-khan007 commented 2 years ago

Issue yet exists , seems returns bool ,how can I check

sla100 commented 2 years ago

@sla100 any news on your OCA?

I received a reply from the person responsible for the declarations. There is hope :)

cjbj commented 2 years ago

@sla100 any news on your OCA?

I received a reply from the person responsible for the declarations. There is hope :)

It was completed: welcome! Now I just need to find time to work with your PRs.

cjbj commented 2 years ago

@sla100 your PRs are being looked at by @pvenkatraman . We will merge the changes to our internal repo and then sync back to GitHub. So don't be surprised if you don't see a normal merge on the PRs.

cjbj commented 2 years ago

This got merged as https://github.com/oracle/node-oracledb/commit/ca165e95b8c9e5ddbaf428550435b361d7b0a0a5 Thank you!