Ignore disconnects when stopping node #690

dsaveliev · 2019-02-26T14:11:35Z

Ignore disconnects when stopping node - fixes #367
This is a port of bitcoin/bitcoin#14670

This problem is related to the "stop" RPC call.
It turns out that sometimes the node stops before it sends the response.
That causes the client to retry the call and lead to "ConnectionRefusedError".

I found out that there is a fix in bitcoin repo (in master branch, actually), related to this bug.
In the nutshell, this changes remove forced exit of libevent loop, which allows
to process all the requests gracefully.

Test feature_shutdown.py wasn't ported because it introduces a bug bitcoin/bitcoin#14670 (comment)
The "improvement" bitcoin/bitcoin#14958 requires RPC method "getrpcinfo" which isn't ported from the bitcoin codebase yet.

There are other floating tests with a similar error (ConnectionRefusedError):
fixes #373
fixes #484
fixes #544

Test runs:
rpc_fundrawtransaction.py - https://travis-ci.com/dsaveliev/unit-e/builds/102691551
feature_notifications.py - https://travis-ci.com/dsaveliev/unit-e/builds/102777885
mempool_packages.py - https://travis-ci.com/dsaveliev/unit-e/builds/102779373
feature_cltv.py - https://travis-ci.com/dsaveliev/unit-e/builds/102784432

Signed-off-by: Dmitry Saveliev [email protected]

scravy · 2019-02-26T14:12:51Z

@dsaveliev There's a new github feature for pull requests. When opening one there is a drop down menu in the green "create pull request" button which allows you to open a "draft" pull request. Feel free to make use of it in the future, it's exactly for this purpose (our very own WIP label stems from a time before that)

.travis.yml

test/functional/test_runner.py

Sending the header "Connection: close" makes libevent close persistent connections (implicit with HTTP 1.1) which cleans the event base when shutdown is requested. Signed-off-by: Dmitry Saveliev <[email protected]>

This (almost) move only ensures the event base loop doesn't exit before HTTP worker threads exit. This way events registered by HTTP workers are processed and not discarded. Signed-off-by: Dmitry Saveliev <[email protected]>

Let event base loop exit cleanly by processing all active and pending events. The call is no longer necessary because closing persistent connections is now properly handled. Signed-off-by: Dmitry Saveliev <[email protected]>

Let HTTP connections to timeout due to inactivity. Let all remaning connections finish sending the response and close. Signed-off-by: Dmitry Saveliev <[email protected]>

Signed-off-by: Dmitry Saveliev <[email protected]>

src/rpc/server.cpp

Signed-off-by: Dmitry Saveliev <[email protected]>

scravy

utACK 5f973f8

Looks good to me. I compared it with the bitcoin pull, and this is pretty much the same fix, minus some changes as it's from different branches, but that's fine.

scravy · 2019-02-28T12:55:17Z

@dsaveliev If you put fixes #367 (instead of just refs ...) into the description the ticket will be closed automatically when merging this pull request. It's nice as it's also advertised in the ui as "this ticket will be closed by this pull request" etc.

thothd

Awesome, would be glad to see these floating tests bold green.
@dsaveliev it's probably fixing bunch of more tests than just #367 ?

dsaveliev · 2019-02-28T13:04:41Z

@dsaveliev it's probably fixing bunch of more tests than just #367 ?

I think this might be so. Right now I'm running through the floating tests in order to find similar cases.

AM5800 · 2019-02-28T13:30:31Z

utACK 5f973f8

Congratulations with the first PR =)

kostyantyn · 2019-02-28T13:31:59Z

I am not sure we should modify commit messages we take from Bitcoin. I didn't check them all but I see we added Signed-off-by block. I think we should just take such commits as is.

scravy · 2019-02-28T13:36:13Z

Please do not squash merge this, I will merge-merge this.

thothd · 2019-02-28T13:36:51Z

I am not sure we should modify commit messages we take from Bitcoin. I didn't check them all but I see we added Signed-off-by block. I think we should just take such commits as is.

@kostyantyn maybe explain what's the issue with it? I think you've done the same for the ECMH commit

dsaveliev · 2019-02-28T13:39:07Z

I am not sure we should modify commit messages we take from Bitcoin. I didn't check them all but I see we added Signed-off-by block. I think we should just take such commits as is.

The problem is that DCO check doesn't pass without signing every single commit, so I was forced to do this.

scravy · 2019-02-28T13:40:41Z

The problem is that DCO check doesn't pass without signing every single commit, so I was forced to do this.

That's just partially true. For exactly these cases you can go into the failing check (follow the "Details" link) and "Set DCO to pass" (there's a button).

kostyantyn · 2019-02-28T13:41:19Z

It's enough to cherry-pick commits. git cherry-pick <commit hash> and push to the branch. Here I see that we also edited commit messages and added the Signed-off-by. I'd suggest just keep such messages are is, without editing them and then add new commits on top of them to correct what is needed.

cornelius · 2019-02-28T13:43:00Z

I am not sure we should modify commit messages we take from Bitcoin. I didn't check them all but I see we added Signed-off-by block. I think we should just take such commits as is.

Adding the Signed-off-by trailer is fine, I think. As a committer you sign off on the patch of the author. This implies that you have checked that the patch is ok to apply to unit-e. That is actually useful information.

kostyantyn

utACK 5f973f8

As soon as we agree on commit messages as I see we didn't do that for other such PRs.

thothd · 2019-02-28T14:00:56Z

It's enough to cherry-pick commits. git cherry-pick <commit hash> and push to the branch. Here I see that we also edited commit messages and added the Signed-off-by. I'd suggest just keep such messages are is, without editing them and then add new commits on top of them to correct what is needed.

I think @dsaveliev cherry picked them, just had to adjust the code a bit since it's quite a recent Bitcoin code
But wherever it’s not the case I agree we shouldn’t touch the commits

dsaveliev · 2019-02-28T14:34:37Z

@thothd @scravy - similar floating tests were added to the PR description.

scravy · 2019-02-28T14:49:09Z

Great, I will merge-merge this then.

dsaveliev added the wip Work in progress which is not supposed to be merged yet label Feb 26, 2019

dsaveliev added this to the 0.1 milestone Feb 26, 2019

dsaveliev self-assigned this Feb 26, 2019

scravy reviewed Feb 26, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

scravy reviewed Feb 26, 2019

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

dsaveliev force-pushed the fix-node-stop-rpc-call branch 3 times, most recently from 14d9038 to 32d95eb Compare February 26, 2019 17:11

dsaveliev changed the title ~~Ignore disconnections when stopping node~~ Ignore disconnects when stopping node Feb 26, 2019

dsaveliev force-pushed the fix-node-stop-rpc-call branch 2 times, most recently from 576fd68 to 22f6210 Compare February 28, 2019 09:12

dsaveliev added bug A problem of existing functionality and removed wip Work in progress which is not supposed to be merged yet labels Feb 28, 2019

promag added 6 commits February 28, 2019 10:39

http: Send "Connection: close" header if shutdown is requested

5740a16

Sending the header "Connection: close" makes libevent close persistent connections (implicit with HTTP 1.1) which cleans the event base when shutdown is requested. Signed-off-by: Dmitry Saveliev <[email protected]>

http: Unlisten sockets after all workers quit

fffe969

This (almost) move only ensures the event base loop doesn't exit before HTTP worker threads exit. This way events registered by HTTP workers are processed and not discarded. Signed-off-by: Dmitry Saveliev <[email protected]>

http: Remove unnecessary event_base_loopexit call

ea8cfb6

Let event base loop exit cleanly by processing all active and pending events. The call is no longer necessary because closing persistent connections is now properly handled. Signed-off-by: Dmitry Saveliev <[email protected]>

http: Remove timeout to exit event loop

1cc43c6

Let HTTP connections to timeout due to inactivity. Let all remaning connections finish sending the response and close. Signed-off-by: Dmitry Saveliev <[email protected]>

rpc: Add wait argument to stop

7b3f9c7

Signed-off-by: Dmitry Saveliev <[email protected]>

qa: Test bitcond shutdown

e9adf37

Signed-off-by: Dmitry Saveliev <[email protected]>

dsaveliev force-pushed the fix-node-stop-rpc-call branch from 22f6210 to e9adf37 Compare February 28, 2019 09:39

scravy requested a review from a team February 28, 2019 10:31

scravy reviewed Feb 28, 2019

View reviewed changes

src/rpc/server.cpp Outdated Show resolved Hide resolved

Remove obsolete comment from "stop" rpc call definition. (#690)

5f973f8

Signed-off-by: Dmitry Saveliev <[email protected]>

dsaveliev requested a review from thothd February 28, 2019 12:38

scravy approved these changes Feb 28, 2019

View reviewed changes

thothd reviewed Feb 28, 2019

View reviewed changes

AM5800 approved these changes Feb 28, 2019

View reviewed changes

kostyantyn approved these changes Feb 28, 2019

View reviewed changes

scravy merged commit 6753dac into dtr-org:master Feb 28, 2019

cmihai mentioned this pull request Feb 28, 2019

feature_notifications failed #373

Closed

dsaveliev mentioned this pull request Mar 6, 2019

fix functional test: feature_proxy.py #5

Closed

Ignore disconnects when stopping node #690

Ignore disconnects when stopping node #690

Uh oh!

Conversation

dsaveliev commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scravy commented Feb 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scravy left a comment

Choose a reason for hiding this comment

Uh oh!

scravy commented Feb 28, 2019

Uh oh!

thothd left a comment

Choose a reason for hiding this comment

Uh oh!

dsaveliev commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AM5800 commented Feb 28, 2019

Uh oh!

kostyantyn commented Feb 28, 2019

Uh oh!

scravy commented Feb 28, 2019

Uh oh!

thothd commented Feb 28, 2019

Uh oh!

dsaveliev commented Feb 28, 2019

Uh oh!

scravy commented Feb 28, 2019

Uh oh!

kostyantyn commented Feb 28, 2019

Uh oh!

cornelius commented Feb 28, 2019

Uh oh!

kostyantyn left a comment

Choose a reason for hiding this comment

Uh oh!

thothd commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsaveliev commented Feb 28, 2019

Uh oh!

scravy commented Feb 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dsaveliev commented Feb 26, 2019 •

edited

Loading

dsaveliev commented Feb 28, 2019 •

edited

Loading

thothd commented Feb 28, 2019 •

edited

Loading