Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors from encodeValue_ are not caught and generate a UnhandledPromiseRejectionWarning #1078

Closed
WaldoJeffers opened this issue Jun 5, 2020 · 4 comments · Fixed by #1208
Closed
Assignees
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@WaldoJeffers
Copy link
Contributor

WaldoJeffers commented Jun 5, 2020

Environment details

  • OS: MacOSX 10.14.6
  • Node.js version: v10.19.0
  • npm version: 6.13.4
  • @google-cloud/spanner version: 5.1.0

Bug description

Errors thrown by common-grpc/service/encodeValue_ like this one (passing undefined as value for a named parameter) are never caught by this function's caller, resulting in Unhandled Promise Rejections. Also, for some reason it seems to crash the underlying Node.js process.

The error thrown in encodeValue_ goes all the way to SessionPool.getReadSession where it fails to be caught:

Steps to reproduce

There are probably different ways to reproduce the error; all you have to do is make the encodeValue_ helper throw. One way to achieve this is to provide an undefined value to a named parameter in a query

  1. Run a query using named parameters, but don't pass a value to the parameter
const q = {
  json: true,
  params: { },
  sql: 'SELECT * FROM tableId WHERE namedParameter = @namedParameter',
};
const [rows] = await db.run(q); // This will lead to an Unhandled Promise rejection

So any attempt to catch the error will fail

const [rows] = await db.run(q).catch(() => console.log('I will never be called'));

But perhaps more unexpectedly, the code will stop running after this line:

const [rows] = await db.run(q).catch(() => console.log('I will never be called'));
console.log('Unreachable code is never called ???');

I am preparing a PR which might help fix the issue; but wanted to file the issue separately in case you prefer to fix the issue differently.

Thanks for your help :)

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Jun 5, 2020
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jun 6, 2020
@skuruppu skuruppu added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Jun 9, 2020
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jun 9, 2020
@skuruppu skuruppu removed the triage me I really want to be triaged. label Jun 9, 2020
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jun 9, 2020
@hengfengli hengfengli added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed triage me I really want to be triaged. labels Jun 9, 2020
@AVaksman
Copy link
Contributor

AVaksman commented Jul 9, 2020

@WaldoJeffers the below snippet worked for me.

    const q = {
        json: true,
        sql: 'SELECT * FROM Singers WHERE SingerId=@id',
    }
    try {
        const [rows] = await database.run(q);
        console.log(rows.length);
        rows.forEach(console.log);
    } catch (error) {
        console.log('I will get called.')
        console.log('Error message => ' + error.message);
    }
    console.log('Continue execution here...');
I will get called.
Error message => 3 INVALID_ARGUMENT: No parameter found for binding: id
Continue execution here...

@WaldoJeffers
Copy link
Contributor Author

WaldoJeffers commented Jul 9, 2020

Hello @AVaksman,

Thanks for your answer. Could try the same thing with

    const q = {
        json: true,
        params : { id: undefined }, // <-- Add this line (assuming both Singers & SingerId really exist)
        sql: 'SELECT * FROM Singers WHERE SingerId=@id',
    }
    try {
        const [rows] = await database.run(q);
        console.log(rows.length);
        rows.forEach(console.log);
    } catch (error) {
        console.log('I will get called.')
        console.log('Error message => ' + error.message);
    }
    console.log('Continue execution here...');

This does result in (node:8165) UnhandledPromiseRejectionWarning: Error: Value of type undefined not recognized. for me

olavloite added a commit to olavloite/nodejs-spanner that referenced this issue Aug 5, 2020
Errors in transaction.runStream(..) were not handled by database.runStream(..), which
could cause Unhandled Promise Rejections.

Fixes googleapis#1078
@olavloite olavloite self-assigned this Aug 5, 2020
@olavloite
Copy link
Contributor

@WaldoJeffers Would you mind also having a look at #1208?

@WaldoJeffers
Copy link
Contributor Author

Hello @olavloite ,

Thanks for your PR. It's probably a much better fix than my PR, but I'm not familiar enough with the codebase to provide feedback on the changes 😬

olavloite added a commit that referenced this issue Aug 23, 2020
* fix: handle potential errors when creating stream

Errors in transaction.runStream(..) were not handled by database.runStream(..), which
could cause Unhandled Promise Rejections.

Fixes #1078

* fix: move error handling to snapshot.runStream

* fix: improve stream error handling

* test: add tests for parameter encoding

* fix: process review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
6 participants