From 488444354b17b5f5feb9c3bd10024a43425cd972 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Tue, 25 Apr 2017 22:52:37 +0000 Subject: [PATCH] lib/db: Don't panic So, when first implementing the database layer I added panics on every unexpected error condition mostly to be sure to flush out bugs and inconsistencies. Then it became sort of standard, and we don't seem to have many bugs here any more so the panics are usually caused by things like checksum errors on read. But it's not an optimal user experience to crash all the time. Here I've weeded out most of the panics, while retaining a few "can't happen" ones like errors on marshalling and write that we really can't recover from. For the rest, I'm mostly treating any read error as "entry didn't exist". This should mean we'll rescan the file and correct the info (if scanning) or treat it as a new file and do conflict handling (when pulling). In some cases things like our global stats may be slightly incorrect until a restart, if a database entry goes suddenly missing during runtime. All in all, I think this makes us a bit more robust and friendly without introducing too many risks for the user. If the database is truly toast, probably many other things on the system will be toast as well... GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4118 --- lib/db/leveldb.go | 6 +- lib/db/leveldb_dbinstance.go | 105 ++++++++++++++++----------------- lib/db/leveldb_transactions.go | 40 ++++++------- 3 files changed, 72 insertions(+), 79 deletions(-) diff --git a/lib/db/leveldb.go b/lib/db/leveldb.go index 47823314..ea5a8843 100644 --- a/lib/db/leveldb.go +++ b/lib/db/leveldb.go @@ -69,13 +69,15 @@ func getFile(db dbReader, key []byte) (protocol.FileInfo, bool) { return protocol.FileInfo{}, false } if err != nil { - panic(err) + l.Debugln("surprise error:", err) + return protocol.FileInfo{}, false } var f protocol.FileInfo err = f.Unmarshal(bs) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + return protocol.FileInfo{}, false } return f, true } diff --git a/lib/db/leveldb_dbinstance.go b/lib/db/leveldb_dbinstance.go index 9d6355dd..75a411e8 100644 --- a/lib/db/leveldb_dbinstance.go +++ b/lib/db/leveldb_dbinstance.go @@ -197,8 +197,16 @@ func (db *Instance) updateFiles(folder, device []byte, fs []protocol.FileInfo, l for _, f := range fs { name := []byte(f.Name) fk = db.deviceKeyInto(fk[:cap(fk)], folder, device, name) + + // Get and unmarshal the file entry. If it doesn't exist or can't be + // unmarshalled we'll add it as a new entry. bs, err := t.Get(fk, nil) - if err == leveldb.ErrNotFound { + var ef FileInfoTruncated + if err == nil { + err = ef.Unmarshal(bs) + } + + if err != nil { if isLocalDevice { localSize.addFile(f) } @@ -212,11 +220,6 @@ func (db *Instance) updateFiles(folder, device []byte, fs []protocol.FileInfo, l continue } - var ef FileInfoTruncated - err = ef.Unmarshal(bs) - if err != nil { - panic(err) - } // The Invalid flag might change without the version being bumped. if !ef.Version.Equal(f.Version) || ef.Invalid != f.Invalid { if isLocalDevice { @@ -262,7 +265,8 @@ func (db *Instance) withHave(folder, device, prefix []byte, truncate bool, fn It // we need to copy it. f, err := unmarshalTrunc(append([]byte{}, dbi.Value()...), truncate) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + continue } if cont := fn(f); !cont { return @@ -286,7 +290,8 @@ func (db *Instance) withAllFolderTruncated(folder []byte, fn func(device []byte, // we need to copy it. err := f.Unmarshal(append([]byte{}, dbi.Value()...)) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + continue } switch f.Name { @@ -315,32 +320,35 @@ func (db *Instance) getGlobal(folder, file []byte, truncate bool) (FileIntf, boo defer t.close() bs, err := t.Get(k, nil) - if err == leveldb.ErrNotFound { - return nil, false - } if err != nil { - panic(err) + return nil, false } var vl VersionList err = vl.Unmarshal(bs) + if err == leveldb.ErrNotFound { + return nil, false + } if err != nil { - panic(err) + l.Debugln("unmarshal error:", k, err) + return nil, false } if len(vl.Versions) == 0 { - l.Debugln(k) - panic("no versions?") + l.Debugln("no versions:", k) + return nil, false } k = db.deviceKey(folder, vl.Versions[0].Device, file) bs, err = t.Get(k, nil) if err != nil { - panic(err) + l.Debugln("surprise error:", k, err) + return nil, false } fi, err := unmarshalTrunc(bs, truncate) if err != nil { - panic(err) + l.Debugln("unmarshal error:", k, err) + return nil, false } return fi, true } @@ -362,11 +370,12 @@ func (db *Instance) withGlobal(folder, prefix []byte, truncate bool, fn Iterator var vl VersionList err := vl.Unmarshal(dbi.Value()) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + continue } if len(vl.Versions) == 0 { - l.Debugln(dbi.Key()) - panic("no versions?") + l.Debugln("no versions:", dbi.Key()) + continue } name := db.globalKeyName(dbi.Key()) @@ -377,22 +386,14 @@ func (db *Instance) withGlobal(folder, prefix []byte, truncate bool, fn Iterator fk = db.deviceKeyInto(fk[:cap(fk)], folder, vl.Versions[0].Device, name) bs, err := t.Get(fk, nil) if err != nil { - l.Debugf("folder: %q (%x)", folder, folder) - l.Debugf("key: %q (%x)", dbi.Key(), dbi.Key()) - l.Debugf("vl: %v", vl) - l.Debugf("vl.Versions[0].Device: %x", vl.Versions[0].Device) - l.Debugf("name: %q (%x)", name, name) - l.Debugf("fk: %q", fk) - l.Debugf("fk: %x %x %x", - fk[keyPrefixLen:keyPrefixLen+keyFolderLen], - fk[keyPrefixLen+keyFolderLen:keyPrefixLen+keyFolderLen+keyDeviceLen], - fk[keyPrefixLen+keyFolderLen+keyDeviceLen:]) - panic(err) + l.Debugln("surprise error:", err) + continue } f, err := unmarshalTrunc(bs, truncate) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + continue } if cont := fn(f); !cont { @@ -408,13 +409,15 @@ func (db *Instance) availability(folder, file []byte) []protocol.DeviceID { return nil } if err != nil { - panic(err) + l.Debugln("surprise error:", err) + return nil } var vl VersionList err = vl.Unmarshal(bs) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + return nil } var devices []protocol.DeviceID @@ -442,11 +445,12 @@ nextFile: var vl VersionList err := vl.Unmarshal(dbi.Value()) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + continue } if len(vl.Versions) == 0 { - l.Debugln(dbi.Key()) - panic("no versions?") + l.Debugln("no versions:", dbi.Key()) + continue } have := false // If we have the file, any version @@ -477,21 +481,14 @@ nextFile: fk = db.deviceKeyInto(fk[:cap(fk)], folder, vl.Versions[i].Device, name) bs, err := t.Get(fk, nil) if err != nil { - var id protocol.DeviceID - copy(id[:], device) - l.Debugf("device: %v", id) - l.Debugf("need: %v, have: %v", need, have) - l.Debugf("key: %q (%x)", dbi.Key(), dbi.Key()) - l.Debugf("vl: %v", vl) - l.Debugf("i: %v", i) - l.Debugf("fk: %q (%x)", fk, fk) - l.Debugf("name: %q (%x)", name, name) - panic(err) + l.Debugln("surprise error:", err) + continue nextVersion } gf, err := unmarshalTrunc(bs, truncate) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + continue nextVersion } if gf.IsInvalid() { @@ -579,7 +576,8 @@ func (db *Instance) checkGlobals(folder []byte, globalSize *sizeTracker) { var vl VersionList err := vl.Unmarshal(dbi.Value()) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + continue } // Check the global version list for consistency. An issue in previous @@ -597,16 +595,15 @@ func (db *Instance) checkGlobals(folder []byte, globalSize *sizeTracker) { continue } if err != nil { - panic(err) + l.Debugln("surprise error:", err) + return } newVL.Versions = append(newVL.Versions, version) if i == 0 { - fi, ok := t.getFile(folder, version.Device, name) - if !ok { - panic("nonexistent global master file") + if fi, ok := t.getFile(folder, version.Device, name); ok { + globalSize.addFile(fi) } - globalSize.addFile(fi) } } diff --git a/lib/db/leveldb_transactions.go b/lib/db/leveldb_transactions.go index 1e54e78e..a4ee60ed 100644 --- a/lib/db/leveldb_transactions.go +++ b/lib/db/leveldb_transactions.go @@ -89,21 +89,14 @@ func (t readWriteTransaction) updateGlobal(folder, device []byte, file protocol. l.Debugf("update global; folder=%q device=%v file=%q version=%d", folder, protocol.DeviceIDFromBytes(device), file.Name, file.Version) name := []byte(file.Name) gk := t.db.globalKey(folder, name) - svl, err := t.Get(gk, nil) - if err != nil && err != leveldb.ErrNotFound { - panic(err) - } + svl, _ := t.Get(gk, nil) // skip error, we check len(svl) != 0 later var fl VersionList var oldFile protocol.FileInfo var hasOldFile bool // Remove the device from the current version list if len(svl) != 0 { - err = fl.Unmarshal(svl) - if err != nil { - panic(err) - } - + fl.Unmarshal(svl) // skip error, range handles success case for i := range fl.Versions { if bytes.Equal(fl.Versions[i].Device, device) { if fl.Versions[i].Version.Equal(file.Version) { @@ -147,11 +140,10 @@ func (t readWriteTransaction) updateGlobal(folder, device []byte, file protocol. // "Greater" in the condition above is just based on the device // IDs in the version vector, which is not the only thing we use // to determine the winner.) + // + // A surprise missing file entry here is counted as a win for us. of, ok := t.getFile(folder, fl.Versions[i].Device, name) - if !ok { - panic("file referenced in version list does not exist") - } - if file.WinsConflict(of) { + if !ok || file.WinsConflict(of) { fl.Versions = insertVersion(fl.Versions, i, nv) insertedAt = i goto done @@ -174,11 +166,11 @@ done: globalSize.removeFile(oldFile) } else if len(fl.Versions) > 1 { // The previous newest version is now at index 1, grab it from there. - oldFile, ok := t.getFile(folder, fl.Versions[1].Device, name) - if !ok { - panic("file referenced in version list does not exist") + if oldFile, ok := t.getFile(folder, fl.Versions[1].Device, name); ok { + // A failure to get the file here is surprising and our + // global size data will be incorrect until a restart... + globalSize.removeFile(oldFile) } - globalSize.removeFile(oldFile) } } } @@ -206,7 +198,8 @@ func (t readWriteTransaction) removeFromGlobal(folder, device, file []byte, glob var fl VersionList err = fl.Unmarshal(svl) if err != nil { - panic(err) + l.Debugln("unmarshal error:", err) + return } removed := false @@ -215,7 +208,8 @@ func (t readWriteTransaction) removeFromGlobal(folder, device, file []byte, glob if i == 0 && globalSize != nil { f, ok := t.getFile(folder, device, file) if !ok { - panic("removing nonexistent file") + // didn't exist anyway, apparently + continue } globalSize.removeFile(f) removed = true @@ -231,11 +225,11 @@ func (t readWriteTransaction) removeFromGlobal(folder, device, file []byte, glob l.Debugf("new global after remove: %v", fl) t.Put(gk, mustMarshal(&fl)) if removed { - f, ok := t.getFile(folder, fl.Versions[0].Device, file) - if !ok { - panic("new global is nonexistent file") + if f, ok := t.getFile(folder, fl.Versions[0].Device, file); ok { + // A failure to get the file here is surprising and our + // global size data will be incorrect until a restart... + globalSize.addFile(f) } - globalSize.addFile(f) } } }