From de3932e8e9a6a9dca01b467436a593f898cbdcf8 Mon Sep 17 00:00:00 2001 From: Mitar Date: Mon, 14 Oct 2024 14:11:59 +0200 Subject: [PATCH] fix: sentinel errors should not have stack traces (#2042) ## Problem While debugging a program which is using badger I noticed that stack traces returned from my code (which uses errors with stack traces) are useless. I found the reason: badger uses sentinel errors (great!) but it creates them with stack traces at the "global init time" (e.g., `var ErrFoo = errors.New(...)`) and not at "return from function time". Because of that I was seeing stack traces like: ``` github.com/dgraph-io/badger/v4.init /go/pkg/mod/github.com/dgraph-io/badger/v4@v4.2.0/compaction.go:41 runtime.doInit1 /usr/local/go/src/runtime/proc.go:6740 runtime.doInit /usr/local/go/src/runtime/proc.go:6707 runtime.main /usr/local/go/src/runtime/proc.go:249 runtime.goexit /usr/local/go/src/runtime/asm_amd64.s:1650 ``` Instead of a stack trace which would show me where the call which is returning sentinel error is made. ## Solution Ideally, one would create all sentinel errors without stack traces (standard errors package works great for that) and then when the error happens in a function, you return `errors.WithStack(ErrFoobar)` instead of just `ErrFoobar`. This means that sentinel error is then annotated with a stack trace at the place where the error happens. But if you do that, then you have to check for sentinel errors with `errors.Is` instead of just comparing errors with equality (`==`). That might be a breaking change for people who are not using `errors.Is` as they should. Because badger's own codebase uses equality instead of `errors.Is` I decided to not do this in this PR, but instead: Just make sentinel errors without stack traces. This allows me to annotate with a stack trace inside my program (otherwise my program does not do so because if finds an existing stack trace and assumes it is a deeper one) and I get at least that part of the stack trace. Equality still works as usual. I suggest this is merged this way and in v5 of badger `errors.WithStack` and required `errors.Is` are introduced. ## Side note Some time ago [I made this errors package for Go](https://gitlab.com/tozd/go/errors) which fixes various issues in `github.com/pkg/errors` and also has direct support for sentinel (in that package called "base") errors. It also has some features badger has in its `y` package for dealing with errors. You could consider adopting that package to have a comprehensive errors handling in badger. --- badger/cmd/bank.go | 3 ++- db.go | 3 ++- errors.go | 57 +++++++++++++++++++++++----------------------- levels.go | 3 ++- manifest.go | 5 ++-- merge.go | 5 ++-- value.go | 5 ++-- y/checksum.go | 4 ++-- y/y.go | 7 +++--- 9 files changed, 47 insertions(+), 45 deletions(-) diff --git a/badger/cmd/bank.go b/badger/cmd/bank.go index 07b2e2564..413be1f38 100644 --- a/badger/cmd/bank.go +++ b/badger/cmd/bank.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "errors" + stderrors "errors" "fmt" "log" "math" @@ -148,7 +149,7 @@ func min(a, b uint64) uint64 { return b } -var errAbandoned = errors.New("Transaction abandonded due to insufficient balance") +var errAbandoned = stderrors.New("Transaction abandonded due to insufficient balance") func moveMoney(db *badger.DB, from, to int) error { return db.Update(func(txn *badger.Txn) error { diff --git a/db.go b/db.go index ba060cc57..6b4cc6932 100644 --- a/db.go +++ b/db.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "encoding/binary" + stderrors "errors" "expvar" "fmt" "math" @@ -1015,7 +1016,7 @@ func (db *DB) batchSetAsync(entries []*Entry, f func(error)) error { return nil } -var errNoRoom = errors.New("No room for write") +var errNoRoom = stderrors.New("No room for write") // ensureRoomForWrite is always called serially. func (db *DB) ensureRoomForWrite() error { diff --git a/errors.go b/errors.go index f5df6d511..ffdac6a24 100644 --- a/errors.go +++ b/errors.go @@ -17,9 +17,8 @@ package badger import ( + stderrors "errors" "math" - - "github.com/pkg/errors" ) const ( @@ -30,97 +29,97 @@ const ( var ( // ErrValueLogSize is returned when opt.ValueLogFileSize option is not within the valid // range. - ErrValueLogSize = errors.New("Invalid ValueLogFileSize, must be in range [1MB, 2GB)") + ErrValueLogSize = stderrors.New("Invalid ValueLogFileSize, must be in range [1MB, 2GB)") // ErrKeyNotFound is returned when key isn't found on a txn.Get. - ErrKeyNotFound = errors.New("Key not found") + ErrKeyNotFound = stderrors.New("Key not found") // ErrTxnTooBig is returned if too many writes are fit into a single transaction. - ErrTxnTooBig = errors.New("Txn is too big to fit into one request") + ErrTxnTooBig = stderrors.New("Txn is too big to fit into one request") // ErrConflict is returned when a transaction conflicts with another transaction. This can // happen if the read rows had been updated concurrently by another transaction. - ErrConflict = errors.New("Transaction Conflict. Please retry") + ErrConflict = stderrors.New("Transaction Conflict. Please retry") // ErrReadOnlyTxn is returned if an update function is called on a read-only transaction. - ErrReadOnlyTxn = errors.New("No sets or deletes are allowed in a read-only transaction") + ErrReadOnlyTxn = stderrors.New("No sets or deletes are allowed in a read-only transaction") // ErrDiscardedTxn is returned if a previously discarded transaction is re-used. - ErrDiscardedTxn = errors.New("This transaction has been discarded. Create a new one") + ErrDiscardedTxn = stderrors.New("This transaction has been discarded. Create a new one") // ErrEmptyKey is returned if an empty key is passed on an update function. - ErrEmptyKey = errors.New("Key cannot be empty") + ErrEmptyKey = stderrors.New("Key cannot be empty") // ErrInvalidKey is returned if the key has a special !badger! prefix, // reserved for internal usage. - ErrInvalidKey = errors.New("Key is using a reserved !badger! prefix") + ErrInvalidKey = stderrors.New("Key is using a reserved !badger! prefix") // ErrBannedKey is returned if the read/write key belongs to any banned namespace. - ErrBannedKey = errors.New("Key is using the banned prefix") + ErrBannedKey = stderrors.New("Key is using the banned prefix") // ErrThresholdZero is returned if threshold is set to zero, and value log GC is called. // In such a case, GC can't be run. - ErrThresholdZero = errors.New( + ErrThresholdZero = stderrors.New( "Value log GC can't run because threshold is set to zero") // ErrNoRewrite is returned if a call for value log GC doesn't result in a log file rewrite. - ErrNoRewrite = errors.New( + ErrNoRewrite = stderrors.New( "Value log GC attempt didn't result in any cleanup") // ErrRejected is returned if a value log GC is called either while another GC is running, or // after DB::Close has been called. - ErrRejected = errors.New("Value log GC request rejected") + ErrRejected = stderrors.New("Value log GC request rejected") // ErrInvalidRequest is returned if the user request is invalid. - ErrInvalidRequest = errors.New("Invalid request") + ErrInvalidRequest = stderrors.New("Invalid request") // ErrManagedTxn is returned if the user tries to use an API which isn't // allowed due to external management of transactions, when using ManagedDB. - ErrManagedTxn = errors.New( + ErrManagedTxn = stderrors.New( "Invalid API request. Not allowed to perform this action using ManagedDB") // ErrNamespaceMode is returned if the user tries to use an API which is allowed only when // NamespaceOffset is non-negative. - ErrNamespaceMode = errors.New( + ErrNamespaceMode = stderrors.New( "Invalid API request. Not allowed to perform this action when NamespaceMode is not set.") // ErrInvalidDump if a data dump made previously cannot be loaded into the database. - ErrInvalidDump = errors.New("Data dump cannot be read") + ErrInvalidDump = stderrors.New("Data dump cannot be read") // ErrZeroBandwidth is returned if the user passes in zero bandwidth for sequence. - ErrZeroBandwidth = errors.New("Bandwidth must be greater than zero") + ErrZeroBandwidth = stderrors.New("Bandwidth must be greater than zero") // ErrWindowsNotSupported is returned when opt.ReadOnly is used on Windows - ErrWindowsNotSupported = errors.New("Read-only mode is not supported on Windows") + ErrWindowsNotSupported = stderrors.New("Read-only mode is not supported on Windows") // ErrPlan9NotSupported is returned when opt.ReadOnly is used on Plan 9 - ErrPlan9NotSupported = errors.New("Read-only mode is not supported on Plan 9") + ErrPlan9NotSupported = stderrors.New("Read-only mode is not supported on Plan 9") // ErrTruncateNeeded is returned when the value log gets corrupt, and requires truncation of // corrupt data to allow Badger to run properly. - ErrTruncateNeeded = errors.New( + ErrTruncateNeeded = stderrors.New( "Log truncate required to run DB. This might result in data loss") // ErrBlockedWrites is returned if the user called DropAll. During the process of dropping all // data from Badger, we stop accepting new writes, by returning this error. - ErrBlockedWrites = errors.New("Writes are blocked, possibly due to DropAll or Close") + ErrBlockedWrites = stderrors.New("Writes are blocked, possibly due to DropAll or Close") // ErrNilCallback is returned when subscriber's callback is nil. - ErrNilCallback = errors.New("Callback cannot be nil") + ErrNilCallback = stderrors.New("Callback cannot be nil") // ErrEncryptionKeyMismatch is returned when the storage key is not // matched with the key previously given. - ErrEncryptionKeyMismatch = errors.New("Encryption key mismatch") + ErrEncryptionKeyMismatch = stderrors.New("Encryption key mismatch") // ErrInvalidDataKeyID is returned if the datakey id is invalid. - ErrInvalidDataKeyID = errors.New("Invalid datakey id") + ErrInvalidDataKeyID = stderrors.New("Invalid datakey id") // ErrInvalidEncryptionKey is returned if length of encryption keys is invalid. - ErrInvalidEncryptionKey = errors.New("Encryption key's length should be" + + ErrInvalidEncryptionKey = stderrors.New("Encryption key's length should be" + "either 16, 24, or 32 bytes") // ErrGCInMemoryMode is returned when db.RunValueLogGC is called in in-memory mode. - ErrGCInMemoryMode = errors.New("Cannot run value log GC when DB is opened in InMemory mode") + ErrGCInMemoryMode = stderrors.New("Cannot run value log GC when DB is opened in InMemory mode") // ErrDBClosed is returned when a get operation is performed after closing the DB. - ErrDBClosed = errors.New("DB Closed") + ErrDBClosed = stderrors.New("DB Closed") ) diff --git a/levels.go b/levels.go index 6bbaf55ca..c787c4463 100644 --- a/levels.go +++ b/levels.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "encoding/hex" + stderrors "errors" "fmt" "math" "math/rand" @@ -1512,7 +1513,7 @@ func tablesToString(tables []*table.Table) []string { return res } -var errFillTables = errors.New("Unable to fill tables") +var errFillTables = stderrors.New("Unable to fill tables") // doCompact picks some table on level l and compacts it away to the next level. func (s *levelsController) doCompact(id int, p compactionPriority) error { diff --git a/manifest.go b/manifest.go index e681ae0a0..2355f281e 100644 --- a/manifest.go +++ b/manifest.go @@ -20,6 +20,7 @@ import ( "bufio" "bytes" "encoding/binary" + stderrors "errors" "fmt" "hash/crc32" "io" @@ -352,8 +353,8 @@ func (r *countingReader) ReadByte() (b byte, err error) { } var ( - errBadMagic = errors.New("manifest has bad magic") - errBadChecksum = errors.New("manifest has checksum mismatch") + errBadMagic = stderrors.New("manifest has bad magic") + errBadChecksum = stderrors.New("manifest has checksum mismatch") ) // ReplayManifestFile reads the manifest file and constructs two manifest objects. (We need one diff --git a/merge.go b/merge.go index 3ec28e90e..1b7333ca3 100644 --- a/merge.go +++ b/merge.go @@ -17,11 +17,10 @@ package badger import ( + stderrors "errors" "sync" "time" - "github.com/pkg/errors" - "github.com/dgraph-io/badger/v4/y" "github.com/dgraph-io/ristretto/z" ) @@ -58,7 +57,7 @@ func (db *DB) GetMergeOperator(key []byte, return op } -var errNoMerge = errors.New("No need for merge") +var errNoMerge = stderrors.New("No need for merge") func (op *MergeOperator) iterateAndMerge() (newVal []byte, latest uint64, err error) { txn := op.db.NewTransaction(false) diff --git a/value.go b/value.go index 6b2f81016..76e06ed42 100644 --- a/value.go +++ b/value.go @@ -19,6 +19,7 @@ package badger import ( "bytes" "context" + stderrors "errors" "fmt" "hash" "hash/crc32" @@ -63,8 +64,8 @@ const ( vlogHeaderSize = 20 ) -var errStop = errors.New("Stop iteration") -var errTruncate = errors.New("Do truncate") +var errStop = stderrors.New("Stop iteration") +var errTruncate = stderrors.New("Do truncate") type logEntry func(e Entry, vp valuePointer) error diff --git a/y/checksum.go b/y/checksum.go index 5a4ac11b6..555c22e32 100644 --- a/y/checksum.go +++ b/y/checksum.go @@ -17,16 +17,16 @@ package y import ( + stderrors "errors" "hash/crc32" "github.com/cespare/xxhash/v2" - "github.com/pkg/errors" "github.com/dgraph-io/badger/v4/pb" ) // ErrChecksumMismatch is returned at checksum mismatch. -var ErrChecksumMismatch = errors.New("checksum mismatch") +var ErrChecksumMismatch = stderrors.New("checksum mismatch") // CalculateChecksum calculates checksum for data using ct checksum type. func CalculateChecksum(data []byte, ct pb.Checksum_Algorithm) uint64 { diff --git a/y/y.go b/y/y.go index 83da2c1d5..f7fe8933c 100644 --- a/y/y.go +++ b/y/y.go @@ -19,6 +19,7 @@ package y import ( "bytes" "encoding/binary" + stderrors "errors" "fmt" "hash/crc32" "io" @@ -30,8 +31,6 @@ import ( "time" "unsafe" - "github.com/pkg/errors" - "github.com/dgraph-io/badger/v4/pb" "github.com/dgraph-io/ristretto/z" ) @@ -39,11 +38,11 @@ import ( var ( // ErrEOF indicates an end of file when trying to read from a memory mapped file // and encountering the end of slice. - ErrEOF = errors.New("ErrEOF: End of file") + ErrEOF = stderrors.New("ErrEOF: End of file") // ErrCommitAfterFinish indicates that write batch commit was called after // finish - ErrCommitAfterFinish = errors.New("Batch commit not permitted after finish") + ErrCommitAfterFinish = stderrors.New("Batch commit not permitted after finish") ) type Flags int