http: Detect remote disconnect #19434
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is a requirement in case we want to support RPC interruption due to connection close. For instance, currently interrupting the command This is also relevant for long calls, especially |
This was referenced Jul 3, 2020
69ca6c7 to
5ea6e50
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones: If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Re-run CI. |
luke-jr
reviewed
Jul 23, 2020
| self.node.getblocktemplate({'longpollid': self.longpollid, 'rules': ['segwit']}) | ||
| try: | ||
| self.node.getblocktemplate({'longpollid': self.longpollid, 'rules': ['segwit']}) | ||
| except: |
This fixes a race between http_request_cb and StopHTTPServer where the work queue is used after release.
5ea6e50 to
578fed1
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Looks like this might be waiting on #19420 |
|
Given that #19420 (base PR) has now been closed, going to close this for now as well. If the base PR is reopened, we could reopen this PR too. |
achow101
added a commit
that referenced
this pull request
Mar 7, 2023
… - 2nd attempt 60978c8 test: Reduce extended timeout on abortnode test (Fabian Jahr) 660bdbf http: Release server before waiting for event base loop exit (João Barbosa) 8c6d007 http: Track active requests and wait for last to finish (João Barbosa) Pull request description: This revives #19420. Since promag is not so active at the moment, I can support this to finally get it merged. The PR is rebased and comments by jonatack have been addressed. Once this is merged, I will also reopen #19434. ACKs for top commit: achow101: ACK 60978c8 stickies-v: re-ACK [60978c8](60978c8) hebasto: ACK 60978c8 Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Mar 7, 2023
… finish - 2nd attempt 60978c8 test: Reduce extended timeout on abortnode test (Fabian Jahr) 660bdbf http: Release server before waiting for event base loop exit (João Barbosa) 8c6d007 http: Track active requests and wait for last to finish (João Barbosa) Pull request description: This revives bitcoin#19420. Since promag is not so active at the moment, I can support this to finally get it merged. The PR is rebased and comments by jonatack have been addressed. Once this is merged, I will also reopen bitcoin#19434. ACKs for top commit: achow101: ACK 60978c8 stickies-v: re-ACK [60978c8](bitcoin@60978c8) hebasto: ACK 60978c8 Tree-SHA512: eef0fe1081e9331b95cfafc71d82f2398abd1d3439dac5b2fa5c6d9c0a3f63ef19adde1c38c88d3b4e7fb41ce7c097943f1815c10e33d165918ccbdec512fe1c
1 task
fanquake
added a commit
that referenced
this pull request
Oct 4, 2023
…lient disconnection 68f23f5 http: bugfix: track closed connection (stickies-v) 084d037 http: log connection instead of request count (stickies-v) 41f9027 http: refactor: use encapsulated HTTPRequestTracker (stickies-v) Pull request description: #26742 significantly increased the http server shutdown speed, but also introduced a bug (#27722 - see #27722 (comment) for steps to reproduce on master) that causes http server shutdown to halt in case of a remote client disconnection. This happens because `evhttp_request_set_on_complete_cb` is never called and thus the request is never removed from `g_requests`. This PR fixes that bug, and improves robustness of the code by encapsulating the request tracking logic. Earlier approaches (#27909, #27245, #19434) attempted to resolve this but [imo are fundamentally unsafe](#27909 (comment)) because of differences in lifetime between an `evhttp_request` and `evhttp_connection`. We don't need to keep track of open requests or connections, we just [need to ensure](#19420 (comment)) that there are no active requests on server shutdown. Because a connection can have multiple requests, and a request can be completed in various ways (the request actually being handled, or the client performing a remote disconnect), keeping a counter per connection seems like the approach with the least overhead to me. Fixes #27722 ACKs for top commit: vasild: ACK 68f23f5 theStack: ACK 68f23f5 Tree-SHA512: dfa711ff55ec75ba44d73e9e6fac16b0be25cf3c20868c2145a844a7878ad9fc6998d9ff62d72f3a210bfa411ef03d3757b73d68a7c22926e874c421e51444d6
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 13, 2023
…emote client disconnection 68f23f5 http: bugfix: track closed connection (stickies-v) 084d037 http: log connection instead of request count (stickies-v) 41f9027 http: refactor: use encapsulated HTTPRequestTracker (stickies-v) Pull request description: bitcoin#26742 significantly increased the http server shutdown speed, but also introduced a bug (bitcoin#27722 - see bitcoin#27722 (comment) for steps to reproduce on master) that causes http server shutdown to halt in case of a remote client disconnection. This happens because `evhttp_request_set_on_complete_cb` is never called and thus the request is never removed from `g_requests`. This PR fixes that bug, and improves robustness of the code by encapsulating the request tracking logic. Earlier approaches (bitcoin#27909, bitcoin#27245, bitcoin#19434) attempted to resolve this but [imo are fundamentally unsafe](bitcoin#27909 (comment)) because of differences in lifetime between an `evhttp_request` and `evhttp_connection`. We don't need to keep track of open requests or connections, we just [need to ensure](bitcoin#19420 (comment)) that there are no active requests on server shutdown. Because a connection can have multiple requests, and a request can be completed in various ways (the request actually being handled, or the client performing a remote disconnect), keeping a counter per connection seems like the approach with the least overhead to me. Fixes bitcoin#27722 ACKs for top commit: vasild: ACK 68f23f5 theStack: ACK 68f23f5 Tree-SHA512: dfa711ff55ec75ba44d73e9e6fac16b0be25cf3c20868c2145a844a7878ad9fc6998d9ff62d72f3a210bfa411ef03d3757b73d68a7c22926e874c421e51444d6
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Builds on #19420, review last commit only.