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

rpcclient: fix crash in http retry handler #1856

Merged
merged 4 commits into from
May 17, 2022
Merged

rpcclient: fix crash in http retry handler #1856

merged 4 commits into from
May 17, 2022

Conversation

bhandras
Copy link
Contributor

@bhandras bhandras commented May 9, 2022

This PR fixes a crash that happens every time when the post errors max retries times.

guggero
guggero approved these changes May 10, 2022
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM, nice fix 🎉

rpcclient/infrastructure.go Show resolved Hide resolved
Roasbeef
Roasbeef reviewed May 10, 2022
View reviewed changes
rpcclient/infrastructure.go Show resolved Hide resolved
rpcclient/infrastructure.go Show resolved Hide resolved
rpcclient/infrastructure.go Outdated
select {
case <-time.After(backoff):

case <-shutdown:
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Roasbeef Roasbeef mentioned this pull request May 11, 2022
Merged
@Roasbeef
Copy link
Member

Tried this on my local machine running while running lightningnetwork/lnd#6345. I still get the panic error, as the response is nil, and all the retries fail since the my bitcoind could actually never stand up (some openssl dynlib thing messed up my bitcoind install).

@coveralls
Copy link

coveralls commented May 11, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2295615882

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 43 (0.0%) changed or added relevant lines in 1 file are covered.
  • 319 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.06%) to 55.001%
Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcclient/infrastructure.go 0 43 0.0%
Files with Coverage Reduction New Missed Lines %
btcutil/psbt/partialsig.go 2 75.0%
btcutil/psbt/utils.go 18 91.41%
btcutil/psbt/partial_output.go 24 61.54%
btcutil/psbt/partial_input.go 45 79.39%
btcutil/psbt/finalizer.go 57 75.92%
rpcclient/infrastructure.go 173 0.0%
Totals Coverage Status
Change from base Build 2303916551: -0.06%
Covered Lines: 26160
Relevant Lines: 47563
💛 - Coveralls

@bhandras bhandras force-pushed the rpcclient-fix branch 2 times, most recently from 52a3fbc to fff3453 Compare May 11, 2022 08:02
@bhandras
Copy link
Contributor Author

Tried this on my local machine running while running lightningnetwork/lnd#6345. I still get the panic error, as the response is nil, and all the retries fail since the my bitcoind could actually never stand up (some openssl dynlib thing messed up my bitcoind install).

In my case the bitcoind wasn't borked but it still crashed after some recurring error that was masked in the retry loop. To be sure we never crash, added coverage for nil result too (will return an "invalid response" error).

@bhandras bhandras requested a review from Roasbeef May 11, 2022 08:04
guggero
guggero approved these changes May 11, 2022
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice!

Roasbeef
Roasbeef reviewed May 12, 2022
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Thanks for the clarifications! Latest changes are looking a lot better, one final comment re trying to pass back more descriptive error.

rpcclient/infrastructure.go Show resolved Hide resolved
@bhandras bhandras requested a review from Roasbeef May 12, 2022 20:10
@bhandras
rpcclient: fix formatting
ebed192
@bhandras
rpclient: fix masked error causing crash after max retries
9665112 
This commit fixes the error that is masked inside the for loop's scope.
Previously after max retries the error didn't leave the for scope and
therefore httpResponse remained nil which in turn resulted in a crash.
@bhandras
rpcclient: fix backoff logic
9babf1f 
This commit removes Sleep() from the rety handler so that the shutdown
request is always respected. Furthermore the maximum retry count is corrected.
@bhandras
rpcclient: save the last error when retrying
97313ac
@bhandras
Copy link
Contributor Author

Thanks for the clarifications! Latest changes are looking a lot better, one final comment re trying to pass back more descriptive error.

ptal

Roasbeef
Roasbeef approved these changes May 17, 2022
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🚵🏼‍♂️

@Roasbeef Roasbeef merged commit cee92e0 into btcsuite:master May 17, 2022
@bhandras bhandras mentioned this pull request Jun 3, 2022
Merged
@chappjc chappjc mentioned this pull request Mar 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@Roasbeef Roasbeef Roasbeef approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bhandras @Roasbeef @coveralls @guggero