Skip to content

Commit

Permalink
storage: don't panic on Iterator.Close
Browse files Browse the repository at this point in the history
Don't panic if Iterator.Close returns a non-nil error. Iteration errors should
already be surfaced through (*pebble.Iterator).Error and
(storage.EngineIterator).Valid. Panicking may crash the node for retriable,
ephemeral I/O errors, especially when reading from sstables over the network.

In a follow up, we should make sure we're still appropriately panicking for
non-retriable errors.

Fix cockroachdb#84396.

Release note (bug fix): fix bug where an ephemeral I/O error could crash a
node.
  • Loading branch information
jbowens committed Jul 14, 2022
1 parent c0ccb6c commit 688bdfd
Showing 1 changed file with 5 additions and 4 deletions.
9 changes: 5 additions & 4 deletions pkg/storage/pebble_iterator.go
Original file line number Diff line number Diff line change
Expand Up @@ -892,10 +892,11 @@ func (p *pebbleIterator) destroy() {
panic("iterator still in use")
}
if p.iter != nil {
err := p.iter.Close()
if err != nil {
panic(err)
}
_ = p.iter.Close()
// TODO(jackson): The error returned by iter.Close() may be an ephemeral
// error, or it may a misuse of the Iterator or corruption. Only swallow
// ephemeral errors (eg, DeadlineExceeded, etc), panic-ing on errors
// that are not known to be ephemeral/retriable.
p.iter = nil
}
// Reset all fields except for the key and option buffers. Holding onto their
Expand Down

0 comments on commit 688bdfd

Please sign in to comment.