Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discard data more liberally on database errors. #339

Merged
merged 2 commits into from
Feb 20, 2018
Merged

Discard data more liberally on database errors. #339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6532eab
Discard data more liberally on database errors.
dbemiller Feb 12, 2018
7ac3c57
Removed logs from tests.
dbemiller Feb 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions stored_requests/backends/db_fetcher/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,23 +49,31 @@ func (fetcher *dbFetcher) FetchRequests(ctx context.Context, ids []string) (map[
}
return nil, []error{err}
}
defer rows.Close()
defer func() {
if err := rows.Close(); err != nil {
glog.Errorf("error closing DB connection: %v", err)
}
}()

reqData := make(map[string]json.RawMessage, len(ids))
var errs []error = nil
for rows.Next() {
var id string
var thisReqData []byte

// Fixes #338?
if err := rows.Scan(&id, &thisReqData); err != nil {
errs = append(errs, err)
return nil, []error{err}
}

reqData[id] = thisReqData
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we fix the problem if we simply put this line into an else, that is only populate reqData[] if there were no errors? Or is there some logic downstream that depends on all IDs being populated if we don't return a nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was actually my first thought too, but... on closer investigation, I don't think so.

I tried to write a failed test to verify it, but sqlmock doesn't let you fake "scan errors". There's a good explanation of why here: DATA-DOG/go-sqlmock#47

I tried to save other types as mock row data, but... it seems like Go is pretty liberal about what it coerces into []byte. Both true and -1 came back as real data, with no Scan error. I'm not sure what if there are other types of data which we could use to force it, either.

Judging by the race condition tickets I linked in #338, though, I think this behavior is caused by something much less straightforward.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yes, dug into them now. If the buffer is being corrupted, that makes subsequent calls suspect. May be able to get away with exiting returning the rows collected prior to the row with the error. But then at that point just tossing everything out may not be a bad response either.

Sounds like we should be able to revisit after go 1.10 is out, if we remember this issue.

}

// Fixes #338?
if rows.Err() != nil {
errs = append(errs, rows.Err())
return nil, []error{rows.Err()}
}

var errs []error
for _, id := range ids {
if _, ok := reqData[id]; !ok {
errs = append(errs, fmt.Errorf(`Stored Request with ID="%s" not found.`, id))
Expand Down
30 changes: 29 additions & 1 deletion stored_requests/backends/db_fetcher/fetcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/DATA-DOG/go-sqlmock"
"regexp"
"testing"
"time"

"github.com/DATA-DOG/go-sqlmock"
)

func TestEmptyQuery(t *testing.T) {
Expand Down Expand Up @@ -165,6 +166,33 @@ func TestContextCancelled(t *testing.T) {
}
}

// Prevents #338?
func TestRowErrors(t *testing.T) {
db, mock, err := sqlmock.New()
if err != nil {
t.Fatalf("Failed to create mock: %v", err)
}
rows := sqlmock.NewRows([]string{"id", "config"})
rows.AddRow("foo", []byte(`{"data":1}`))
rows.AddRow("bar", []byte(`{"data":2}`))
rows.RowError(1, errors.New("Error reading from row 1"))
mock.ExpectQuery(".*").WillReturnRows(rows)
fetcher := &dbFetcher{
db: db,
queryMaker: successfulQueryMaker("SELECT id, config FROM my_table WHERE id IN (?)"),
}
data, errs := fetcher.FetchRequests(context.Background(), []string{"foo", "bar"})
if len(errs) != 1 {
t.Fatalf("Expected 1 error. Got %v", errs)
}
if errs[0].Error() != "Error reading from row 1" {
t.Errorf("Unexpected error message: %v", errs[0].Error())
}
if len(data) != 0 {
t.Errorf("We should not return data from the row where the error occurred.")
}
}

func newFetcher(rows *sqlmock.Rows, query string, args ...driver.Value) (sqlmock.Sqlmock, *dbFetcher, error) {
db, mock, err := sqlmock.New()
if err != nil {
Expand Down