queryStream hangs if sql has a logical error

[awoodworth]

1. What versions are you using?

Oracle Database 11g Enterprise Edition Release 11.2.0.4.0 - 64bit Production

> process.platform
'darwin'
> process.version
'v14.15.0'
> process.arch
'x64'
> require('oracledb').versionString
'5.0.0'
> require('oracledb').oracleClientVersionString
'12.1.0.2.0'

2. Is it an error or a hang or a crash?

crash I believe, but it causes my application to hang based on implementation

3. What error(s) or behavior you are seeing?

The sql hangs/crashes when using queryStream if there's a logical error in the select/result set, but only if the error occurs after the first row. This does not occur when using execute.

4. Include a runnable Node.js script that shows the problem.

const oracledb = require('oracledb');
const fs = require('fs');
const path = require('path');
const { Transform, pipeline } = require('stream');

// oracledb.initOracleClient({ libDir: '/usr/local/oracle/instantclient_19_3' });

let pool;
let conn;
const sql = 'select 1 from dual union all select 1 from dual union all select 1/0 from dual';

(async () => {
  try {
    pool = await oracledb.createPool({
      user: '',
      password: '',
      connectString: '',
    });
    conn = await pool.getConnection();

    const queryStream = conn.queryStream(sql, {}, { outFormat: oracledb.OUT_FORMAT_OBJECT });
    queryStream.on('error', console.error);
    queryStream.on('close', () => {
      console.log('queryStream CLOSED!');
      await conn.close();
      await pool.close(10);
    });

    // create other streams
    const transformStream = new Transform({
      objectMode: true,
      transform(chunk, encoding, callback) {
        console.log(chunk);
        return callback(null, JSON.stringify(chunk, null, 2));
      },
    });
    const writeStream = fs.createWriteStream(path.join(__dirname, 'output.json'));
    writeStream.on('finish', () => {
      console.log('DONE!');
    });

    // run the stream
    pipeline(queryStream, transformStream, writeStream, (err) => {
      if (err) console.error(err);
    });
  } catch (err) {
    console.error(err);
  }
})();

[cjbj]

Interesting.

One probably-not-so-feasible-in-practice workaround is to set oracledb.prefetchRows to the number of records expected.

[niallosul]

@cjbj - I've been working with @awoodworth on this one, and I think I see a possible fix. If you look at the catch block at line 102 in queryStream.js, it just destroys the resultSet without emitting the error. I changed that catch block to this:

   catch (err) {
   	this.emit('error',err)
   	this.destroy(err);
   }

And I started seeing the error in our application, and was able to handle accordingly... Not sure if that's a legitimate fix, but could be a starting point?

[cjbj]

Excellent - thanks for the analysis. That seems feasible but will need a closer look to confirm.

[niallosul]

@cjbj - I took a closer look at this, and I think I've figured out why _destroy doesn't emit the error in it's callback. There's an await on line 47 that never resolves. Since the _read function jumps to the catch block on a getRow error , and that's after the rs is created but before fetching is set to false or '_doneFetching' is emitted.

I was able to fix this a couple of different ways:

catch (err) {
  this._fetching = false;
  this.destroy(err);
}

or

catch (err) {
  this.destroy(err);
  this.emit('_doneFetching');
} 

Again, not sure if either of these are legitimate permanent fixes, but they solve the problem we were seeing

[cjbj]

From quick testing, this._fetching = false; seems simplest. But let us do some more analysis. Maybe a small refactoring will be best.

[anthony-tuininga]

This is the patch that I think should solve your problem as well as address a small buglet: namely that after a query stream has been read once, that the result set could then be fetched independently -- which is not intended! Can you try this patch and make sure it works for you, too? Thanks!

diff --git a/lib/queryStream.js b/lib/queryStream.js
index 08ddc720..09efc99a 100644
--- a/lib/queryStream.js
+++ b/lib/queryStream.js
@@ -89,11 +89,6 @@ class QueryStream extends Readable {
       this._fetching = true;
       this._resultSet._allowGetRowCall = true;
       const row = await this._resultSet.getRow();
-      this._fetching = false;
-      if (!this._resultSet) {
-        this.emit('_doneFetching');
-        return;
-      }
       if (row) {
         this.push(row);
       } else {
@@ -101,6 +96,13 @@ class QueryStream extends Readable {
       }
     } catch (err) {
       this.destroy(err);
+    } finally {
+      this._fetching = false;
+      if (this.resultSet) {
+        this._resultSet._allowGetRowCall = false;
+      } else {
+        this.emit('_doneFetching');
+      }
     }
   }
 

[awoodworth]

@anthony-tuininga this fixes the issue I was seeing with query stream. Thanks!

[niallosul]

@anthony-tuininga - is there a specific patch version with this fix in it, or do we need to wait until it's released?

[anthony-tuininga]

@niallosul, glad to hear it worked for you, too! @cjbj will push this patch to this repository after having some internal testing performed. I'll let him comment on when it might be included in a release!

[cjbj]

Let me get back to you on scheduling. For us to build a full release takes a bit of coordination and release testing. Luckily, since this is a JS patch, you can easily apply it as part of a package.json run script yourselves.

[niallosul]

Thanks @cjbj - I thought there must be a way to do that... I'm not sure I know exactly how to set it up though - would you happen to have an example?

[cjbj]

@niallosul Try this test. Download this gist as issue1391.patch and create a package.json file:

{
  "name": "cjtest",
  "version": "1.0.0",
  "private": true,
  "description": "Test app",
  "scripts": {
    "start": "node index.js",
    "postinstall": "cd node_modules/oracledb && patch -p1 < ../../issue1391.patch"
  },
  "keywords": [
    "myapp"
  ],
  "dependencies": {
    "oracledb": "^5.2"
  },
  "author": "CJ",
  "license": "MIT"
}

When you run npm install the patch will be applied (on macOS & Linux, at least).

Regarding scheduling, we want to see if we can tidy up one new feature (two phase commit support) in a few weeks and do a 5.3 release. If it looks like non-Node.js project will delay us, then we can look at doing a 5.2.1 patch - if you really, really need it!