-
Notifications
You must be signed in to change notification settings - Fork 122
loopd+loopdb: add timeout to DB open #374
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.
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
Conversation
yyforyongyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
carlaKC
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition! Just some nittyness around where the user-instructive/ terminal-specific error should live.
loopd/macaroons.go
Outdated
| loopdb.DefaultLoopDBTimeout, macaroons.IPLockChecker, | ||
| ) | ||
| if err == bbolt.ErrTimeout { | ||
| return fmt.Errorf("error while trying to open %s/%s: "+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great explanatory error!
Just wondering where these errors should live. startMacaroonService and NewBoltSwapStore are both called from our initialize function for the daemon. WDYT about bubbling the bbolt.ErrTimeout all the way up (or wrap it so that we can also have the db name), and then we can have a single descriptive error in the daemon?
If we do it like that, the terminal-specific instructions could also be handled in terminal by checking the response for StartAsSubserver's response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've implemented your suggestion. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
To make sure we don't just block for forever if another Loop daemon process is already running, we add a timeout and error out if obtaining the unique file lock fails after 5 seconds.
carlaKC
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating! LGTM
To make sure we don't just block for forever if another Loop daemon
process is already running, we add a timeout and error out if obtaining
the unique file lock fails after 5 seconds.
Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes