fix: Properly close stmt and conn when using idb

[kadler]

Deleting the statement object doesn't free the underlying resources,
causing memory leaks. Instead, we need to close both the connection and
the statement to free these resources. We can let the runtime delete the
objects as well.

Fixes #230

[kadler]

This might not be 100% correct for sync mode, since I don't know how errors are returned in that case (exceptions?)

[abmusse]

This might not be 100% correct for sync mode, since I don't know how errors are returned in that case (exceptions?)

Yup prepareSync would throw an error here.

This won't give you the chance to properly close things out with cleanup().

Looks we can get around this by passing a callback to prepareSync. That way we can check for an error just like the async case

[kadler]

I've changed the whole design. Since everything throws errors, wrap the whole thing in try/finally blocks. This way we don't eve need the function, since the clean up code only happens in one place.

The only downfall with this design is that cleanup will not be done until after the user's callback chain has returned to us.

[abmusse]

I tried running through the tests with:

$ npm test test/functional/commandsFunctional.js

    Uncaught Error: SQLSTATE=PAERR SQLCODE=8013 The SQL Statment handler is not initialized.
      at ../istoredp.js:77:16

I'm trying to figure out why.

[abmusse]

@kadler

The JavaScript try…catch mechanism cannot be used to intercept errors generated by asynchronous APIs.

See https://nodejs.org/api/errors.html#errors_error_first_callbacks

Running the command test with verbose mode on we can see:

============
INPUT XML
============
<?xml version='1.0'?><myscript><cmd exec='rexx' error='fast'>RTVJOBA USRLIBL(?) SYSLIBL(?)</cmd></myscript>
============
OUTPUT XML
============
SQLConnect(0): conn obj [1805037d0] handler [2]
creating statement
stmt is dbstmt {}
Prepare().
SQLFreeStmt: stmth 3 [SQL_DROP]
SQLFreeStmt(0)
SQLDisconnect: conn obj [1805037d0] handler [2]
SQLFreeConnect: conn obj [1805037d0] handler [2]
SQLFreeConnect[0]
SQLPrepare(-2): call QXMLSERV.iPLUG512K(?,?,?,?)
calling bind
BindParamAsync().
      1) calls CL command using idb transport


  0 passing (114ms)
  1 failing

  1) iSh, iCmd, iQsh, Functional Tests
       iCmd()
         calls CL command using idb transport:
     Uncaught Error: SQLSTATE=PAERR SQLCODE=8013 The SQL Statment handler is not initialized.
      at ../istoredp.js:83:16

Looks like the finally block runs before prepare finishes causing the statement handle to be invalid SQLPrepare(-2).

[abmusse]

I suggest we avoid try..catch mechanism when in async mode.

[kadler]

Ugh, that makes sense. The fact that the whole thing is wrapped in an exception handler and the code throws is what lead me to that. Now I wonder if the code is currently broken.

[kadler]

I moved the try/finally around the sync mode block and in the async block I used the same way I had before.

[abmusse]

Ugh, that makes sense. The fact that the whole thing is wrapped in an exception handler and the code throws is what lead me to that. Now I wonder if the code is currently broken.

Throwing an error withing the async callback is definitely wrong.

Furthermore v0.x did not report errors back to the user at all in the iConn.run() callback.

Having error first callbacks in Connection.run() is one of the key features of 1.x.

[abmusse]

I moved the try/finally around the sync mode block and in the async block I used the same way I had before.

Those changes fixed the issue.

[kadler]

Ugh, that makes sense. The fact that the whole thing is wrapped in an exception handler and the code throws is what lead me to that. Now I wonder if the code is currently broken.

Throwing an error withing the async callback is definitely wrong.

Furthermore v0.x did not report errors back to the user at all in the iConn.run() callback.

Yeah, I think that's all it's doing in this case: the exception is thrown and then caught and printed out. The exception shouldn't get back to the user - if they use async mode, they either they didn't get an error and the callback is called or they did and the callback is not. This is bonkers, but I don't know how to fix it without changing behavior, which is why we did what we did in v1.0. My goal wasn't to change that, but to fix the problem at hand.

[abmusse]

LGTM

[kadler]

@abmusse can you please make a v0.1.7 release?

[abmusse]

@kadler

I believe you meant v0.1.7 right?

After making the release on github also publish on npm?

[kadler]

Yep. Caught my mistake a hair too late.