qa: Drop RPC connection if --usecli #15350
Conversation
|
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. |
|
The travis output: 🤔 |
Oh crap that's still four time slower, better than five admittedly, but not by much. |
|
@MarcoFalke where did you get that? Compare these (same travis job): master (fa2a69f) - https://travis-ci.org/bitcoin/bitcoin/jobs/489678103 this branch - https://travis-ci.org/bitcoin/bitcoin/jobs/489313879 |
|
BTW, locally, with or without And suppose this doesn't fix the slow down, it's still a valid change. |
|
Agree that this change should probably be fine |
|
@MarcoFalke on master, the same job (.9) gives: |
|
Hmm, so this could be due to the sanitizer overhead per process. |
|
@MarcoFalke I think there are lots of things that can make execution times drift, travis machine, load, etc.. Locally, with |
|
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 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 |
|
Removed |
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
|
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. |
|
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 |
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
6 participants
Drop the RPC connection used in
TestNode.wait_for_rpc_connectionif--usecliis set. If the connection is kept and not used theConnection: closeheader 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.