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

Database not found and other errors emitted when they shouldn't be #2106

Open
BobDickinson opened this issue Sep 11, 2024 · 1 comment · May be fixed by #2107
Open

Database not found and other errors emitted when they shouldn't be #2106

BobDickinson opened this issue Sep 11, 2024 · 1 comment · May be fixed by #2107
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

@BobDickinson
Copy link

Environment details
OS: MacOS Sonomia 14.6.1
Node.js version: 20.9.0
npm version: 10.1.0
@google-cloud/spanner version: 7.14.0

Steps to reproduce

Note: I was somewhat reluctant to file this because I couldn't easily create a repro case. But I think it's clear from looking at the code that this is broken.

In a process that will handle and complain about unprocessed errors (such as a Jest test), get a Spanner database object representing a database that does not exist (for example, to test for existence with .exist()). Exit the process. There will be an unprocessed error, which in the case of Jest, will cause your test run to fail.

There is an attempt in the session pool code to prevent this exact scenario:

this._fill().catch(err => {

    this._fill().catch(err => {
      // Ignore `Database not found` error. This allows a user to call instance.database('db-name')
      // for a database that does not yet exist with SessionPoolOptions.min > 0.
      if (
        isDatabaseNotFoundError(err) ||
        isInstanceNotFoundError(err) ||
        isCreateSessionPermissionError(err) ||
        isDefaultCredentialsNotSetError(err) ||
        isProjectIdNotSetInEnvironmentError(err)
      ) {
        return;
      }
      this.emit('error', err);
    });

The problem is that _fill() doesn't throw exceptions, it emits them as errors. So those errors are emitted as 'error' events by _fill() and the code in the this._fill().catch() is never hit.

It should be noted that there will be other session pool listeners attached, in particular any database objects will have a bound emitter that forwards these errors, so simply handling the errors as the exceptions are handled in open() won't work. I'm having trouble following the logic and understanding the significant entanglement of the various objects involved, so there is no PR immediately obvious to me. Hopefully the maintainers will be able to see that there was a clear intention to this code (well intentioned - I definitely don't need an error event for non-existence, often significantly after the fact, when using exist()) that is no longer being satisfied.

The only workaround I could find was to handle all events on all database objects (since they seem to be variously recycled and share the underlying session pool) and ignore the database not found errors (whether they were related to my original attempt to test for existence or not), which is not ideal. The fact that this event will be sent to some future database object at an unpredictable time is also not ideal (for example, after creating a database, which then does exist).

@BobDickinson BobDickinson added 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. labels Sep 11, 2024
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Sep 11, 2024
@alkatrivedi alkatrivedi linked a pull request Sep 11, 2024 that will close this issue
@alkatrivedi
Copy link
Contributor

Hi @BobDickinson, thanks for pointing it out. I have raised a PR to fix it.

@alkatrivedi alkatrivedi self-assigned this Sep 11, 2024
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
Development

Successfully merging a pull request may close this issue.

2 participants