Skip to content

Conversation

@promag
Copy link
Contributor

@promag promag 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 2018-12-improve-shutdown-test branch from 410ce75 to 406204f Compare December 14, 2018 12:03
@fanquake fanquake added the Tests label Dec 14, 2018
@promag promag force-pushed the 2018-12-improve-shutdown-test branch from 406204f to e2b0fec Compare December 14, 2018 12:04
@promag promag force-pushed the 2018-12-improve-shutdown-test branch 2 times, most recently from b3f27ea to eeb8e04 Compare December 14, 2018 15:56
@promag promag force-pushed the 2018-12-improve-shutdown-test branch from eeb8e04 to 9be0dce Compare January 2, 2019 15:11
laanwj added a commit that referenced this pull request Jan 14, 2019
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 2018-12-improve-shutdown-test branch from 9be0dce to 49a79d4 Compare January 15, 2019 09:56
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@maflcko
Copy link
Member

maflcko 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 2018-12-improve-shutdown-test branch from 49a79d4 to 4412a59 Compare January 16, 2019 12:02
@laanwj
Copy link
Member

laanwj 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
laanwj added a commit that referenced this pull request Jan 16, 2019
…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 2018-12-improve-shutdown-test branch January 16, 2019 12:10
scravy added a commit to dtr-org/unit-e that referenced this pull request Feb 28, 2019
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 <[email protected]>
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…on separate connections

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

Pull request description:

  Fixes the error bitcoin#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 bitcoin#14670 (comment).

Tree-SHA512: 77feb8628d3b9c025ec0cf83565d4d6680cad4fb182fc93a65df8b573f3e799ba4c44e06d9001dd8a375ca0b1ee17f10e66c3902b6256d0ae2acbc64539185d7
vijaydasmp pushed a commit to vijaydasmp/dash that referenced this pull request Sep 12, 2021
…on separate connections

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

Pull request description:

  Fixes the error bitcoin#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 bitcoin#14670 (comment).

Tree-SHA512: 77feb8628d3b9c025ec0cf83565d4d6680cad4fb182fc93a65df8b573f3e799ba4c44e06d9001dd8a375ca0b1ee17f10e66c3902b6256d0ae2acbc64539185d7
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants