lib / pq

Pure Go Postgres driver for database/sql
https://pkg.go.dev/github.com/lib/pq
MIT License
8.86k stars 908 forks source link

db.Close() fails when a query operation (ExecContext) is cancelled #1115

Closed fho closed 1 year ago

fho commented 1 year ago

When an ExecContext() operation is cancelled via the context, a following call to db.Close() fails with an net.ErrClosed (use of closed network connection) error. If the context for a query is cancelled, I do not expect to cause it an error when calling db.Close() afterwards. Everything happened as intended.

When the context is cancelled the connection is closed. When db.Close() is called, conn.Close() is called for the closed connection again. conn.Close() tries to sent cn.sendSimpleMessage('X') on the closed connection which fails with net.ErrClosed.

The root cause seems that lib/pq is closing the connection without the knowledge of the database/sql layer, when the context is cancelled. To signal that the context was cancelled ctx.Err() is and need to be returned by lib/pq. This means we can not signal via the return code at the same time that the connection should not be reused. I think because of that lib/pq closes the connection without database/sql knowledge.

Normally database/sql takes care about closing connections. When it runs driver operations (queries) and ErrBadConn is returned, it calls conn.Close and removes the connection from the pool. This can not happen in the ctx cancel scenario currently.

The right fix for it seems to implement the driver.Validator interface for Conn. database/sql calls conn.IsValid() after an operation. When the context was cancelled, conn.Isvalid() would return false and database/sql would then take care about closing the connection and removing it from the pool. This might also allow to simplify error handling in other places in lib/pq too.

If there is interest and chance to get it merged I could implement the change.

This happened on:

This can be reproduced with:

package main

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

    _ "github.com/lib/pq"
)

func main() {
    db, err := sql.Open("postgres", "postgres://postgres:@172.25.0.2/")
    if err != nil {
        log.Fatal(err)
    }

    ctx, cancel := context.WithCancel(context.Background())
    go func() {
        defer cancel()
        time.Sleep(time.Second)
    }()

    _, err = db.ExecContext(ctx, "SELECT pg_sleep(300)")
    if err.Error() != "pq: canceling statement due to user request" {
        log.Fatalf("Unexpected error: %s", err)
    }

    err = db.Close()
    if err != nil {
        log.Fatalf("close failed with: %s", err)
    }
}
fho commented 1 year ago

This was fixed in https://github.com/lib/pq/pull/1079