Discard data more liberally on database errors. #339
Conversation
| if err := rows.Scan(&id, &thisReqData); err != nil { | ||
| errs = append(errs, err) | ||
| return nil, []error{err} | ||
| } | ||
|
|
||
| reqData[id] = thisReqData |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Merging this per the new process. No complaints from the second reviewer in > 5 days. |
* Discard data more liberally on database errors. * Removed logs from tests.
* Discard data more liberally on database errors. * Removed logs from tests.
Fixes #338 ...hopefully. It's tough to say for sure, because it's not really reproducible... but it seems like this should help.