qa: Drop RPC connection if --usecli

promag commented at 0:05 am on February 6, 2019: member

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 e98a9eede2fb48ff33a020acc888cbcd83e24bbf in a follow up, however it changes the shutdown behavior.

qa: Drop RPC connection if --usecli 6440e61375

fanquake added the label Tests on Feb 6, 2019

promag commented at 0:37 am on February 6, 2019: member

@jnewbery @sdaftuar

MarcoFalke commented at 3:07 pm on February 6, 2019: member

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 commented at 3:09 pm on February 6, 2019: member

The travis output:

0...
1wallet_multiwallet.py                 | ✓ Passed  | 20 s
2wallet_multiwallet.py --usecli        | ✓ Passed  | 80 s
3...

:thinking:

laanwj commented at 6:44 pm on February 6, 2019: member

The travis output:

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

promag commented at 7:19 pm on February 6, 2019: member

@MarcoFalke where did you get that?

Compare these (same travis job):

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

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

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

0wallet_multiwallet.py                 | ✓ Passed  | 50 s
1wallet_multiwallet.py --usecli        | ✓ Passed  | 54 s

promag commented at 7:24 pm on February 6, 2019: member

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 commented at 7:30 pm on February 6, 2019: member

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

Agree that this change should probably be fine

promag commented at 7:53 pm on February 6, 2019: member

From here: https://travis-ci.org/bitcoin/bitcoin/jobs/489313883#L3812 @MarcoFalke on master, the same job (.9) gives:

0wallet_multiwallet.py                 | ✓ Passed  | 50 s
1wallet_multiwallet.py --usecli        | ✓ Passed  | 319 s

MarcoFalke commented at 8:01 pm on February 6, 2019: member

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

promag commented at 8:05 pm on February 6, 2019: member

@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 commented at 11:51 pm on February 6, 2019: member

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 commented at 2:46 pm on February 7, 2019: member

Removed Fixes [#15309](/bitcoin-bitcoin/15309/) from OP.

MarcoFalke referenced this in commit d83d607943 on Feb 7, 2019

MarcoFalke merged this on Feb 7, 2019

MarcoFalke closed this on Feb 7, 2019

promag deleted the branch on Feb 7, 2019

jnewbery commented at 3:39 pm on February 7, 2019: member

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 commented at 3:48 pm on February 7, 2019: member

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

MarcoFalke commented at 3:49 pm on February 7, 2019: member

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 commented at 6:39 pm on February 11, 2019: member

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.

Munkybooty referenced this in commit 4b85569591 on Aug 24, 2021

Munkybooty referenced this in commit 44ef8736d7 on Sep 1, 2021

Munkybooty referenced this in commit f90d11e8dc on Sep 5, 2021

Munkybooty referenced this in commit 4d64d44663 on Sep 7, 2021

Munkybooty referenced this in commit ee34a9c6e7 on Sep 7, 2021

Munkybooty referenced this in commit 060ca49dbb on Sep 9, 2021

Munkybooty referenced this in commit b115016807 on Sep 10, 2021

DrahtBot locked this on Dec 16, 2021

qa: Drop RPC connection if –usecli #15350