Skip to content

Commit

Permalink
Don't panic if driverRows.Next is called after Close
Browse files Browse the repository at this point in the history
Previously this would panic with an index out of range error. Which was
misleading as the indexing itself wasn't the problem. The problem was
that fetch returns a nil error if the error channel was closed, which
led the Next function to believe there was more data ready to be
processed, but this was not the case.

The error channel is only closed if Close is called on driverRows or
driverStmt, so this panic was caused by a premature closing of the rows
or statement, but that hard to tell from the panic message.

This commit changes removes the panic and instead returns an io.EOF
error if we detect that fetch was called on a closed statement or rows.
  • Loading branch information
Thomas de Zeeuw authored and losipiuk committed Jan 30, 2023
1 parent e7589ed commit 6d954f2
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 1 deletion.
32 changes: 32 additions & 0 deletions trino/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ package trino
import (
"context"
"database/sql"
"database/sql/driver"
"errors"
"flag"
"io"
"log"
"net/http"
"os"
Expand Down Expand Up @@ -527,6 +529,36 @@ func TestIntegrationQueryParametersSelect(t *testing.T) {
}
}

func TestIntegrationQueryNextAfterClose(t *testing.T) {
// NOTE: This is testing invalid behaviour. It ensures that we don't
// panic if we call driverRows.Next after we closed the driverStmt.

ctx := context.Background()
conn, err := (&Driver{}).Open(*integrationServerFlag)
defer conn.Close()

stmt, err := conn.(driver.ConnPrepareContext).PrepareContext(ctx, "SELECT 1")
if err != nil {
t.Fatalf("Failed preparing query: %v", err)
}

rows, err := stmt.(driver.StmtQueryContext).QueryContext(ctx, []driver.NamedValue{})
if err != nil {
t.Fatalf("Failed running query: %v", err)
}
defer rows.Close()

stmt.Close() // NOTE: the important bit.

var result driver.Value
if err := rows.Next([]driver.Value{result}); err != nil {
t.Fatalf("unexpected result: %+v, no error was expected", err)
}
if err := rows.Next([]driver.Value{result}); err != io.EOF {
t.Fatalf("unexpected result: %+v, expected io.EOF", err)
}
}

func TestIntegrationExec(t *testing.T) {
db := integrationOpen(t)
defer db.Close()
Expand Down
6 changes: 5 additions & 1 deletion trino/trino.go
Original file line number Diff line number Diff line change
Expand Up @@ -1144,7 +1144,11 @@ func (qr *driverRows) fetch() error {
return nil
}
case err = <-qr.stmt.errors:
if err == context.Canceled {
if err == nil {
// Channel was closed, which means the statement
// or rows were closed.
err = io.EOF
} else if err == context.Canceled {
qr.Close()
}
qr.err = err
Expand Down

0 comments on commit 6d954f2

Please sign in to comment.