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: Drop RPC connection if --usecli #15350

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
6 participants
@promag
Copy link
Member

commented Feb 6, 2019

Drop the RPC connection used in TestNode.wait_for_rpc_connection if --usecli is set. If the connection is kept and not used the Connection: close header is never sent and so the connection only closes due to timeout (30 sec).

It might be sensible to revert e98a9ee in a follow up, however it changes the shutdown behavior.

@fanquake fanquake added the Tests label Feb 6, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Concept ACK. It makes no sense to persist the bitcoin-cli connection, since a new process has to be spun up for each call anyway.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

The travis output:

...
wallet_multiwallet.py                 | ✓ Passed  | 20 s
wallet_multiwallet.py --usecli        | ✓ Passed  | 80 s
...

🤔

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

The travis output:

Oh crap that's still four time slower, better than five admittedly, but not by much.

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@MarcoFalke where did you get that?

Compare these (same travis job):

master (fa2a69f) - https://travis-ci.org/bitcoin/bitcoin/jobs/489678103

wallet_multiwallet.py                 | ✓ Passed  | 16 s
wallet_multiwallet.py --usecli        | ✓ Passed  | 282 s

this branch - https://travis-ci.org/bitcoin/bitcoin/jobs/489313879

wallet_multiwallet.py                 | ✓ Passed  | 50 s
wallet_multiwallet.py --usecli        | ✓ Passed  | 54 s
@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

BTW, locally, with or without --usecli the time isn's very different.

And suppose this doesn't fix the slow down, it's still a valid change.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

From here: https://travis-ci.org/bitcoin/bitcoin/jobs/489313883#L3812

Agree that this change should probably be fine

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

From here: https://travis-ci.org/bitcoin/bitcoin/jobs/489313883#L3812

@MarcoFalke on master, the same job (.9) gives:

wallet_multiwallet.py                 | ✓ Passed  | 50 s
wallet_multiwallet.py --usecli        | ✓ Passed  | 319 s
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Hmm, so this could be due to the sanitizer overhead per process.

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

@MarcoFalke I think there are lots of things that can make execution times drift, travis machine, load, etc..

Locally, with --usecli I always get a lot of time on master, and with this change it's always similar to run without --usecli.

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

I think we should be closing all http/rpc connections when shutting down, rather than waiting for the client to close. This seems like a workaround for the narrow case that a client has initiated a stop rpc command -- but if the node shuts down due to configuration error, hardware error, etc we should similarly not be waiting for connections to time out before shutting down, I think.

FYI I believe the test I've included in #15305 is unnecessarily slow because of the long delay in the node shutting down on its own (ie not in response to an rpc stop call).

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Removed Fixes #15309 from OP.

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 7, 2019

Merge bitcoin#15350: qa: Drop RPC connection if --usecli
6440e61 qa: Drop RPC connection if --usecli (João Barbosa)

Pull request description:

  Drop the RPC connection used in `TestNode.wait_for_rpc_connection` if `--usecli` is set. If the connection is kept and not used the `Connection: close` header is never sent and so the connection only closes due to timeout (30 sec).

  It might be sensible to revert e98a9ee in a follow up, however it changes the shutdown behavior.

Tree-SHA512: 2a8ee68b82ab612a556390aae521379e592c39ea0a7855a119282e6fe4cbf02ecafe7a5e2ee37d480f2c0600fa64791117a80fecc7bbe6bbb354107972b3b320

@MarcoFalke MarcoFalke merged commit 6440e61 into bitcoin:master Feb 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@promag promag deleted the promag:2019-01-fixusecli branch Feb 7, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

I've had a look at the code change here, and it seems to be a reasonable workaround.

However, I think the problem that @sdaftuar talks about here: #15350 (comment) needs to be addressed. It seems a shame to 'fix' the test that revealed this problem before fixing the underlying issue in bitcoind.

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@jnewbery this doesn't fix the test introduced by @sdaftuar.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

I don't care enough about the underlying issue to have an opinion on it, but this test fix makes sense regardless of any Bitcoin Core changes. I think there is no use in setting self.rpc to non-null when we are supposed to run with the cli. The code this way looks slightly cleaner.

@promag

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

It seems a shame to 'fix' the test that revealed this problem before fixing the underlying issue in bitcoind

For reference, #15363 is an attempt to fix the shutdown problem found in #15350.

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.