diff --git a/weed/filer/abstract_sql/abstract_sql_store.go b/weed/filer/abstract_sql/abstract_sql_store.go index 14a552315d..fdbb7a8adb 100644 --- a/weed/filer/abstract_sql/abstract_sql_store.go +++ b/weed/filer/abstract_sql/abstract_sql_store.go @@ -428,6 +428,7 @@ func (store *AbstractSqlStore) ListDirectoryPrefixedEntries(ctx context.Context, } defer rows.Close() + stoppedEarly := false for rows.Next() { var name string var data []byte @@ -452,10 +453,20 @@ func (store *AbstractSqlStore) ListDirectoryPrefixedEntries(ctx context.Context, } if !resEachEntryFunc { + stoppedEarly = true break } } + // rows.Next also returns false when the result set fails partway, for + // example on a lost connection; that must not read as the end of the + // directory. A listing the callback stopped on purpose has no such error + // to report (rows.Err would return a canceled context's error instead). + if err == nil && !stoppedEarly { + if rowsErr := rows.Err(); rowsErr != nil { + err = fmt.Errorf("list %s: %w", dirPath, rowsErr) + } + } return lastFileName, err } diff --git a/weed/filer/abstract_sql/abstract_sql_store_test.go b/weed/filer/abstract_sql/abstract_sql_store_test.go index 027c5b308e..7c2f1ad3d3 100644 --- a/weed/filer/abstract_sql/abstract_sql_store_test.go +++ b/weed/filer/abstract_sql/abstract_sql_store_test.go @@ -2,10 +2,13 @@ package abstract_sql import ( "context" + "errors" "reflect" "testing" + "time" "github.com/DATA-DOG/go-sqlmock" + "github.com/seaweedfs/seaweedfs/weed/filer" "github.com/seaweedfs/seaweedfs/weed/util" ) @@ -140,3 +143,74 @@ func TestBucketHooksSkipInvalidBucketNames(t *testing.T) { t.Error(err) } } + +type listGen struct{ SqlGenerator } + +func (listGen) GetSqlListExclusive(string) string { return "SELECT name, meta" } +func (listGen) GetSqlListInclusive(string) string { return "SELECT name, meta" } + +// A listing that loses its connection partway through must report the error, +// not return the rows read so far as if the directory ended there. +func TestListDirectoryEntriesReportsRowIterationError(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + defer db.Close() + + meta, err := (&filer.Entry{}).EncodeAttributesAndChunks() + if err != nil { + t.Fatal(err) + } + connLost := errors.New("connection reset by peer") + mock.ExpectQuery("SELECT name, meta").WillReturnRows( + sqlmock.NewRows([]string{"name", "meta"}). + AddRow("a.txt", meta). + AddRow("b.txt", meta). + RowError(1, connLost)) + + store := &AbstractSqlStore{SqlGenerator: listGen{}, DB: db} + var listed []string + _, err = store.ListDirectoryEntries(context.Background(), "/dir", "", true, 100, func(entry *filer.Entry) (bool, error) { + listed = append(listed, entry.Name()) + return true, nil + }) + if !errors.Is(err, connLost) { + t.Fatalf("ListDirectoryEntries error = %v, want %v (listed %v)", err, connLost, listed) + } +} + +// A listing the callback stops on purpose is not a failure, even if the +// request context is canceled before the rows are closed. +func TestListDirectoryEntriesStoppedByCallbackWithCanceledContext(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + defer db.Close() + + meta, err := (&filer.Entry{}).EncodeAttributesAndChunks() + if err != nil { + t.Fatal(err) + } + mock.ExpectQuery("SELECT name, meta").WillReturnRows( + sqlmock.NewRows([]string{"name", "meta"}).AddRow("a.txt", meta).AddRow("b.txt", meta)) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + store := &AbstractSqlStore{SqlGenerator: listGen{}, DB: db} + _, err = store.ListDirectoryEntries(ctx, "/dir", "", true, 100, func(entry *filer.Entry) (bool, error) { + cancel() + // database/sql closes the rows on a background goroutine; once the + // connection is back in the pool, rows.Err reports the cancellation. + for deadline := time.Now().Add(5 * time.Second); db.Stats().InUse > 0; time.Sleep(time.Millisecond) { + if time.Now().After(deadline) { + t.Fatal("the canceled listing still holds its connection") + } + } + return false, nil + }) + if err != nil { + t.Fatalf("ListDirectoryEntries error = %v, want nil", err) + } +}