snowflakedb / gosnowflake

Go Snowflake Driver
Apache License 2.0
297 stars 124 forks source link

SNOW-1563083: database/sql driver caches cancelled context #1186

Closed joellubi closed 2 months ago

joellubi commented 3 months 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? github.com/snowflakedb/gosnowflake v1.10.1

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

    uname -mrsv 
    Darwin 23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:17:33 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6031 arm64
  3. What version of GO are you using? run go version in your console

    go version    
    go version go1.22.3 darwin/arm64
  4. Server version: 8.27.1

  5. What did you do?

If a *sql.DB instance is opened with sql.OpenDB(connector), then cancelling the context provided to db.QueryContext after the query has completed will result in warnings getting logged that the context is already closed when "recycling" the connection. I've reproduced a minimal example below, but the real-world scenario this comes up in is if errgroup is used to dispatch multiple queries concurrently. Once you Wait() for the group to finish, the group's context is cancelled. The problem is that the driver currently attempts to reuse this context in the call to defer db.Close() later when the function exits.

Based on my investigation, this seems like it has to do with the db connection pooling that Go's database/sql provides automatically. If you uncomment the line db.SetMaxIdleConns(0), then the error does not occur. I believe this is because it prevents Go from caching the connection.

In order for database/sql connections to be persist-able within the cache, they should not store the context from the query that spawned them since it may have a shorter lifetime than the connection if it is reused. The docs indicate this as well:

type Connector interface { ... // The provided context.Context is for dialing purposes only // (see net.DialContext) and should not be stored or used for // other purposes. ... Connect(context.Context) (Conn, error) ... }


package main

import ( "context" "database/sql" "fmt" "log"

"github.com/snowflakedb/gosnowflake"

)

var uri = "REDACTED"

func main() { ctxCancel, cancel := context.WithCancel(context.Background())

config, err := gosnowflake.ParseDSN(uri)
if err != nil {
    log.Fatal(err)
}

connector := gosnowflake.NewConnector(gosnowflake.SnowflakeDriver{}, *config)
db := sql.OpenDB(connector)
defer db.Close()
// db.SetMaxIdleConns(0)

if err := printQueryResult(ctxCancel, db, "SELECT 'hello'"); err != nil {
    log.Fatal(err)
}
cancel()

}

func printQueryResult(ctx context.Context, db *sql.DB, query string) error { rows, err := db.QueryContext(ctx, query) if err != nil { return err } defer rows.Close()

for rows.Next() {
    var val string
    if err := rows.Scan(&val); err != nil {
        return err
    }
    fmt.Println(val)
}
return nil

}


7. What did you expect to see?

**Expected**

go run main.go hello


**Actual**

go run main.go hello ERRO[0000]connection.go:275 gosnowflake.(*snowflakeConn).Close context canceled



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

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

9. What is your Snowflake account identifier, if any? (Optional)
sfc-gh-dszmolka commented 3 months ago

hi and thank you for raising this issue with us; additionally providing all these details and the reproduction is very much appreciated ! we'll take a look

connelld-dpsk12 commented 2 months ago

hi and thank you for raising this issue with us; additionally providing all these details and the reproduction is very much appreciated ! we'll take a look

Any update @sfc-gh-dszmolka ?

sfc-gh-dszmolka commented 2 months ago

I'm keeping all issues updated when there's progress on them - at this moment, i have no update. Issues are addressed by their respective teams in terms of the impact they have. If this issue requires more attention and perhaps causing bigger impact for your company using Snowflake, please raise it with Snowflake Support too.

This repo is constantly monitored, however we cannot guarantee any specific timeline for resolution in advance. Thank you for your understanding!

sfc-gh-dszmolka commented 2 months ago

https://github.com/snowflakedb/gosnowflake/pull/1196 merged - thank you for the contribution! will be released with the next upcoming driver release

sfc-gh-dszmolka commented 2 months ago

released with gosnowflake v.1.11.1