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.

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

qa: Remove race between connecting and shutdown on separate connections #14958

Merged
merged 1 commit into from Jan 16, 2019

Conversation

Projects
None yet
4 participants
@promag
Copy link
Member

commented Dec 14, 2018

Fixes the error #14670 (comment) reported by @ken2812221.

There is a race between RPC stop and another concurrent call in the test framework. The connection must be established and the command waitfornewblock running before calling stop.

See also #14670 (comment).

@promag promag force-pushed the promag:2018-12-improve-shutdown-test branch Dec 14, 2018

@fanquake fanquake added the Tests label Dec 14, 2018

@promag promag force-pushed the promag:2018-12-improve-shutdown-test branch Dec 14, 2018

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

@promag promag force-pushed the promag:2018-12-improve-shutdown-test branch 2 times, most recently Dec 14, 2018

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

@promag promag force-pushed the promag:2018-12-improve-shutdown-test branch Jan 2, 2019

laanwj added a commit that referenced this pull request Jan 14, 2019

Merge #14982: rpc: Add getrpcinfo command
a0ac154 doc: Add getrpcinfo release notes (João Barbosa)
251a91c qa: Add tests for getrpcinfo (João Barbosa)
d0730f5 rpc: Add getrpcinfo command (João Barbosa)
068a8fc rpc: Track active commands (João Barbosa)
bf43832 rpc: Remove unused PreCommand signal (João Barbosa)

Pull request description:

  The new `getrpcinfo` command exposes details of the RPC interface. The details can be configuration properties or runtime values/stats.

  This can be particular useful to coordinate concurrent functional tests (see #14958 from where this was extracted).

Tree-SHA512: 7292cb6087f4c429973d991aa2b53ffa1327d5a213df7d6ba5fc69b01b2e1a411f6d1609fed9234896293317dab05f65064da48b8f2b4a998eba532591d31882

@promag promag force-pushed the promag:2018-12-improve-shutdown-test branch Jan 15, 2019

@@ -20,8 +20,14 @@ def set_test_params(self):

def run_test(self):
node = get_rpc_proxy(self.nodes[0].url, 1, timeout=600, coveragedir=self.nodes[0].coverage_dir)
# Force connection establishment by executing a dummy command.
node.getblockcount()

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jan 15, 2019

Member

Is this required given that we wait anyway until node.waitfornewblock is called, which in turn should set up the connection?

This comment has been minimized.

Copy link
@promag

promag Jan 15, 2019

Author Member

For the test to pass this is not required, but this is here to note that there's still a race after connection establishment. Let me know if it's not important.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

utACK 49a79d454284df7758f83ccd3121afb3e6799342

@promag promag changed the title qa: Remove race between conneting and shutdown on separate connections qa: Remove race between connecting and shutdown on separate connections Jan 16, 2019

@promag promag force-pushed the promag:2018-12-improve-shutdown-test branch to 4412a59 Jan 16, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

utACK 4412a59
No difference with 49a79d4 except for commit msg.

@laanwj laanwj merged commit 4412a59 into bitcoin:master Jan 16, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jan 16, 2019

Merge #14958: qa: Remove race between connecting and shutdown on sepa…
…rate connections

4412a59 qa: Remove race between connecting and shutdown on separate connections (João Barbosa)

Pull request description:

  Fixes the error #14670 (comment) reported by @ken2812221.

  There is a race between RPC stop and another concurrent call in the test framework. The connection must be established and the command `waitfornewblock` running before calling `stop`.

  See also #14670 (comment).

Tree-SHA512: 77feb8628d3b9c025ec0cf83565d4d6680cad4fb182fc93a65df8b573f3e799ba4c44e06d9001dd8a375ca0b1ee17f10e66c3902b6256d0ae2acbc64539185d7

@promag promag deleted the promag:2018-12-improve-shutdown-test branch Jan 16, 2019

scravy added a commit to dtr-org/unit-e that referenced this pull request Feb 28, 2019

Merge pull request #690 from dsaveliev/fix-node-stop-rpc-call
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 removes 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

Signed-off-by: Dmitry Saveliev <dima@thirdhash.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.