Skip to content

rpcclient: fix crash in http retry handler #1856

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
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 🎉

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

case <-shutdown:
Copy link
Member

@Roasbeef Roasbeef May 10, 2022

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

Roasbeef commented May 11, 2022

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 2305632096

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 41 (0.0%) changed or added relevant lines in 1 file are covered.
  • 168 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 55.049%
Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcclient/infrastructure.go 0 41 0.0%
Files with Coverage Reduction New Missed Lines %
rpcclient/infrastructure.go 168 0.0%
Totals Coverage Status
Change from base Build 2303916551: -0.01%
Covered Lines: 26438
Relevant Lines: 48026
💛 - 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

bhandras commented May 11, 2022

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.

@bhandras bhandras requested a review from Roasbeef May 12, 2022 20:10
bhandras added 4 commits May 13, 2022 15:35
@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

bhandras commented May 13, 2022

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

@Roasbeef Roasbeef Roasbeef approved these changes

@guggero guggero guggero 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