Skip to content

[db] move panic handling from goroutine to the parent function#1153

Merged
ahrtr merged 1 commit intoetcd-io:mainfrom
VihasMakwana:move-panics-to-main-routine
Feb 26, 2026
Merged

[db] move panic handling from goroutine to the parent function#1153
ahrtr merged 1 commit intoetcd-io:mainfrom
VihasMakwana:move-panics-to-main-routine

Conversation

@VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Feb 25, 2026

What does this PR do?

Previously, we listened on ech inside a goroutine and triggered a panic if an error was received. Since the panic occurred within the goroutine, it cannot be recovered by the parent function by design.

This PR changes the flow as follows:

  1. Call recursivelyCheckBucket in a goroutine
  2. Wait for any errors in the parent function and panic, if any error was received.

This way, the panic happens outside the goroutine and can be recovered.

Testing

Tested following program with a corrupted DB file and recover() works as expected

package main

import (
	"fmt"

	"go.etcd.io/bbolt"
)

func main() {
	defer func() {
		if r := recover(); r != nil {
			fmt.Println("Recovered from:", r)
		}
	}()

	db, _ := bbolt.Open("PATH_TO_DB", 0666, bbolt.DefaultOptions)
	defer db.Close()
}

receiver_filelog_.tgz

Issues

Closes #1011

@VihasMakwana VihasMakwana force-pushed the move-panics-to-main-routine branch from ac5e2a7 to cb454cb Compare February 25, 2026 11:34
@VihasMakwana VihasMakwana requested a review from rdner February 25, 2026 11:35
@VihasMakwana VihasMakwana force-pushed the move-panics-to-main-routine branch from cb454cb to 5030853 Compare February 25, 2026 11:35
Copy link

@rdner rdner left a comment

Choose a reason for hiding this comment

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

LGTM if we're going to keep the panic behavior.

But is it even necessary to panic here at all?

Why can't we handle it as an error and propagate it to the consumer letting them to decide what to do with it? Panic seems to be overkill from my point of view.

@ahrtr
Copy link
Member

ahrtr commented Feb 25, 2026

/ok-to-test

@ahrtr
Copy link
Member

ahrtr commented Feb 25, 2026

Please also add a changelog item in this PR under https://github.com/etcd-io/bbolt/blob/main/CHANGELOG/CHANGELOG-1.5.md

@VihasMakwana
Copy link
Contributor Author

@ahrtr Done! 😄

@VihasMakwana VihasMakwana force-pushed the move-panics-to-main-routine branch from 1b4d5d5 to 9f4d8fc Compare February 25, 2026 18:56
@ahrtr
Copy link
Member

ahrtr commented Feb 25, 2026

can you please squash/merge the first two commits?

Signed-off-by: VihasMakwana <121151420+VihasMakwana@users.noreply.github.com>
@VihasMakwana VihasMakwana force-pushed the move-panics-to-main-routine branch from 9f4d8fc to b191c99 Compare February 26, 2026 05:10
@VihasMakwana
Copy link
Contributor Author

@ahrtr Done!

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

Thank you!

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, rdner, VihasMakwana

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahrtr ahrtr merged commit 1f5a036 into etcd-io:main Feb 26, 2026
42 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Panic due to failed assertion

4 participants