SNOW-1159847: Missing error event emissions

[GabrielLomba]

Hi! The SDK uses the Connection class as its default class and it extends EventEmitter. However, it does not actually behave like one as it does not emit any event, errors included. For instance, in heartbeat request errors, the SDK just logs them instead of emitting them. Shouldn't we be emitting internal errors through the EventEmitter API? If not, why inherit it at all?

For a concrete example of how the lack of error event emissions is a bit confusing, it created an issue on our end as we're using a Knex SF dialect to manage a connection pool and it relies on error events to dispose a connection (can be seen here). Since the SDK does not emit these events, the connection will stay in the pool even though it shouldn't since it is not usable. In the end, the connection was in a disconnected state and we ended up getting countless Unable to perform operation using terminated connection errors.

It would be great if the class would forward the errors so the users can handle them!

[sfc-gh-dszmolka]

hi and thank you for submitting this issue with us. I believe we can try and tackle it in 2 parts:

1. the actual issue - Unable to perform operation using terminated connection . snowflake-sdk relies on generic-pool for Connection Pool implementation and thus all its configuration is available in Snowflake Node.js driver too, and unfortunately, all its default settings too. Which means by default, the connections in the pool are neither kept alive, nor getting rid of upon expiration.

You could try to address to either adding the Snowflake-specific keepalive mechanism or you could also decide to not keep the connections alive but instead turn on the generic-pool idle connection detection and eviction mechanism. You can find a working example for both in this comment.
Both should work automatically by either not letting the connection becoming invalid on the Snowflake side, or gracefully tear them down from the driver side.

edit: also depending on the situaiton, isValidAsync() can be called perhaps to test whether a particular connection is good for sending Snowflake queries over it.

2. Connection doesn't emit any Events despite being an EventEmitter - this remark is on point and at this moment, I could not say why it was implemented like this, but I see it's already there in the very first version 6 years ago. Probably there were plans to implement events but we never got there.

Anyways, this could be indeed considered an improvement request for the driver and we'll take a look and consider.

[sfc-gh-dszmolka]

Also actually we do seem to emit some events based on Connection events (loadcomplete, etc), including error on some events in row stream. Not everywhere across Connection though and it can be definitely reviewed.

[GabrielLomba]

@sfc-gh-dszmolka Thanks for the update! I haven't read the code deeply enough to assert with confidence that a Connection instance will never emit anything but it's not immediately obvious either since the events you're mentioning belong to both RowStream and Chunk classes. Either way, we agree on that more errors can be emitted.

As for your pool usage remark, we use the Knex to build queries / manage the connection pool currently. We don't consider changing it for now since it's been working fine for us for the most part. The fix we applied on our end was changing the connection validity check from a simple truthy check here to use connection.isUp() instead. We considered isValidAsync() but since it does a heartbeat request under the hood and we have lots of requests, we figured it would slow down operations + increase resource usage (many ongoing heartbeat requests). Even though isUp() is not as accurate, it works most of the time and we can just retry on scenarios the server returns an error.

[sfc-gh-dszmolka]

my bad on the Knex part; seeing the example from their readme

import * as knex from "knex";
import { SnowflakeDialect } from "knex-snowflake-dialect";

export const Snowflake = knex({
  client: SnowflakeDialect,
  debug: true,
  connection: "snowflake://myuser:[email protected]/mydb?warehouse=MY_WAREHOUSE",
  pool: {
    min: 1,
    max: 1
  }
});

as it was somewhat similar to the options generic-pool exposes, I was under the impression it can just be used here too. Apparently; not.
Glad to her you also found a workaround for checking the connection validity. isUp() is admittedly flaky, but if it works better in your particular situation, why not use it.

For the improvement effort of reviewing and improving events emitted from Connection, I'll keep this thread posted if we decide to take on the enhancement - this can take a while so thank you in advance for bearing with us.