Database not found and other errors emitted when they shouldn't be #2106
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.
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:
nodejs-spanner/src/session-pool.ts
Line 571 in 992baaa
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 thethis._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 usingexist()
) 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).
The text was updated successfully, but these errors were encountered: