Closed awoodworth closed 2 years ago
Interesting.
One probably-not-so-feasible-in-practice workaround is to set oracledb.prefetchRows
to the number of records expected.
@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?
Excellent - thanks for the analysis. That seems feasible but will need a closer look to confirm.
@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
From quick testing, this._fetching = false;
seems simplest. But let us do some more analysis. Maybe a small refactoring will be best.
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');
+ }
}
}
@anthony-tuininga this fixes the issue I was seeing with query stream. Thanks!
@anthony-tuininga - is there a specific patch version with this fix in it, or do we need to wait until it's released?
@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!
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.
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?
@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!
Thanks @cjbj - we should be able to work with this until your next release
This fix in now available in node-oracledb 5.3. Thanks for reporting the issue.
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.