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

Jump to bottom

handle badger iterator panics on app stop #1355

Merged
merged 1 commit into from
Jul 1, 2024
Merged

handle badger iterator panics on app stop #1355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 17 additions & 3 deletions pkg/lib/localstore/objectstore/badger_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package objectstore

import (
"fmt"

"github.com/dgraph-io/badger/v4"

"github.com/anyproto/anytype-heart/util/badgerhelper"
Expand Down Expand Up @@ -35,6 +37,12 @@ func iterateKeysByPrefixBatchedTx(
batchSize int,
processKeysFn func(keys [][]byte) error,
) error {
var err error
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
}
}()
Comment on lines +40 to +45
Copy link

Choose a reason for hiding this comment

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

Ensure err is returned if a panic occurs in iterateKeysByPrefixBatchedTx.

The defer function correctly captures and wraps the panic in an error message, but the err variable should be returned if a panic is detected.

	defer func() {
		if r := recover(); r != nil {
			err = fmt.Errorf("badger iterator panic: %v", r)
+			return
		}
	}()
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var err error
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
}
}()
var err error
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
return
}
}()

opts := badger.DefaultIteratorOptions
opts.PrefetchValues = false
opts.Prefix = prefix
Expand All @@ -50,7 +58,7 @@ func iterateKeysByPrefixBatchedTx(
count++

if count == batchSize {
err := processKeysFn(batch)
err = processKeysFn(batch)
if err != nil {
return err
}
Expand All @@ -60,7 +68,7 @@ func iterateKeysByPrefixBatchedTx(
}

if count > 0 {
err := processKeysFn(batch)
err = processKeysFn(batch)
if err != nil {
return err
}
Expand All @@ -70,6 +78,12 @@ func iterateKeysByPrefixBatchedTx(
}

func iterateKeysByPrefixTx(txn *badger.Txn, prefix []byte, processKeyFn func(key []byte)) error {
var err error
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
}
}()
Comment on lines +81 to +86
Copy link

Choose a reason for hiding this comment

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

Ensure err is returned if a panic occurs in iterateKeysByPrefixTx.

The defer function correctly captures and wraps the panic in an error message, but the err variable should be returned if a panic is detected.

	defer func() {
		if r := recover(); r != nil {
			err = fmt.Errorf("badger iterator panic: %v", r)
+			return
		}
	}()

Committable suggestion was skipped due to low confidence.

opts := badger.DefaultIteratorOptions
opts.PrefetchValues = false
opts.Prefix = prefix
Expand All @@ -80,5 +94,5 @@ func iterateKeysByPrefixTx(txn *badger.Txn, prefix []byte, processKeyFn func(key
key := iter.Item().KeyCopy(nil)
processKeyFn(key)
}
return nil
return err
}
9 changes: 8 additions & 1 deletion pkg/lib/localstore/objectstore/queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,16 @@ func (s *dsObjectStore) getInjectedResults(details *types.Struct, score float64,
func (s *dsObjectStore) queryRaw(filter func(g *types.Struct) bool, order database.Order, limit int, offset int) ([]database.Record, error) {
var (
records []database.Record
err error
)

err := s.db.View(func(txn *badger.Txn) error {
defer func() {
Copy link

Choose a reason for hiding this comment

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

Use keyed fields in struct literals.

The github.com/anyproto/anytype-heart/pkg/lib/database.FilterAllIn struct literal uses unkeyed fields. This can lead to maintainability issues.

- optionFilter := database.FilterAllIn{relationKey, listValue.GetListValue()}
+ optionFilter := database.FilterAllIn{
+	Key: relationKey,
+	Value: listValue.GetListValue(),
+}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
defer func() {
defer func() {
optionFilter := database.FilterAllIn{
Key: relationKey,
Value: listValue.GetListValue(),
}

if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
}
}()

Comment on lines +96 to +104
Copy link

Choose a reason for hiding this comment

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

Ensure err is returned if a panic occurs in queryRaw.

The defer function correctly captures and wraps the panic in an error message, but the err variable should be returned if a panic is detected.

	defer func() {
		if r := recover(); r != nil {
			err = fmt.Errorf("badger iterator panic: %v", r)
+			return
		}
	}()
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
err error
)
err := s.db.View(func(txn *badger.Txn) error {
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
}
}()
err error
)
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
return
}
}()

err = s.db.View(func(txn *badger.Txn) error {
opts := badger.DefaultIteratorOptions
opts.PrefetchValues = false
opts.Prefix = pagesDetailsBase.Bytes()
Expand Down
5 changes: 5 additions & 0 deletions pkg/lib/localstore/stores.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,11 @@ func GetKeysByIndex(index Index, txn *badger.Txn, val interface{}, limit int) ([
}

func GetKeys(txn *badger.Txn, prefix string, limit int) []string {
defer func() {
if r := recover(); r != nil {
log.Errorf("badger iterator panic: %v", r)
}
}()
Comment on lines +241 to +245
Copy link

Choose a reason for hiding this comment

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

Ensure the panic handling is effective in GetKeys.

The defer function correctly captures and logs the panic, but it would be better to also return an error if a panic is detected.

	defer func() {
		if r := recover(); r != nil {
			log.Errorf("badger iterator panic: %v", r)
+			err = fmt.Errorf("badger iterator panic: %v", r)
+			return
		}
	}()
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
defer func() {
if r := recover(); r != nil {
log.Errorf("badger iterator panic: %v", r)
}
}()
defer func() {
if r := recover(); r != nil {
log.Errorf("badger iterator panic: %v", r)
err = fmt.Errorf("badger iterator panic: %v", r)
return
}
}()

iter := txn.NewIterator(badger.IteratorOptions{
Prefix: []byte(prefix),
PrefetchValues: false,
Expand Down
8 changes: 7 additions & 1 deletion util/persistentqueue/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,13 @@ func NewBadgerStorage[T Item](db *badger.DB, badgerPrefix []byte, factoryFunc Fa

func (s *badgerStorage[T]) List() ([]T, error) {
var items []T
err := s.db.View(func(txn *badger.Txn) error {
var err error
defer func() {
if r := recover(); r != nil {
err = fmt.Errorf("badger iterator panic: %v", r)
}
}()
Comment on lines +37 to +42
Copy link

Choose a reason for hiding this comment

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

Ensure err is returned if a panic occurs.

The defer function correctly captures and wraps the panic in an error message, but the err variable should be returned if a panic is detected.

	defer func() {
		if r := recover(); r != nil {
			err = fmt.Errorf("badger iterator panic: %v", r)
+			return
		}
	}()

Committable suggestion was skipped due to low confidence.

err = s.db.View(func(txn *badger.Txn) error {
it := txn.NewIterator(badger.IteratorOptions{
PrefetchSize: 100,
PrefetchValues: true,
Expand Down
Loading