snowflakedb / gosnowflake

Go Snowflake Driver
Apache License 2.0
300 stars 125 forks source link

SNOW-1806123: Context isn't being propagated into `authenticateWithConfig` #1244

Open aldld opened 4 hours ago

aldld commented 4 hours ago

Please answer these questions before submitting your issue. In order to accurately debug the issue this information is required. Thanks!

  1. What version of GO driver are you using? 1.11.2

  2. What operating system and processor architecture are you using? N/A

  3. What version of GO are you using? 1.22

4.Server version:* E.g. 1.90.1 You may get the server version by running a query:

SELECT CURRENT_VERSION();
  1. What did you do?

We've run into some cases where some of our customers are having issues establishing Snowflake database connections. In particular, we set a context deadline of e.g. 120 seconds, however, these calls end up running for much longer than they should be (around 354 seconds) before failing.

In the source code for OpenWithConfig, I see that we make a call to authenticateWithConfig(sc). In particular, we don't pass in the incoming ctx. Instead, we end up using sc.ctx, which gets set to context.Background() in buildSnowflakeConn. Am I missing something, or does this mean that there are places where the driver simply doesn't respect context deadlines?

  1. What did you expect to see?

Calls to OpenWithConfig should respect the incoming context deadline.

  1. Can you set logging to DEBUG and collect the logs?

    https://community.snowflake.com/s/article/How-to-generate-log-file-on-Snowflake-connectors

    Before sharing any information, please be sure to review the log and remove any sensitive information.

aldld commented 4 hours ago

Looking bit more broadly, is there any reason why we even store a context on the snowflakeConn, if it's just going to be context.Background()? If the goal is to store a context that can be reused but is detached from the initial request's context, something likecontext.WithoutCancel(ctx)` might be more appropriate.

One additional consequence is that because sc.ctx is missing values from any outside context, and because it's used in a lot of logging calls, that means that any logging hooks that callers have registered won't have access to context values that are set outside of the driver. In those cases though, ideally we'd be using the context provided on the individual QueryContext or ExecContext call, not one that's stored from when the connection was first established.