[qa] Ensure bitcoind processes are cleaned up when tests end #12904

pull sdaftuar wants to merge 1 commits into bitcoin:master from sdaftuar:2018-04-always-kill-bitcoind changing 3 files +17 −0

sdaftuar commented at 3:09 PM on April 6, 2018: member

When tests fail (such as due to a bug in the test, race condition, etc), it's possible that we could follow code paths that bypass our normal node shutdown that occurs in TestNode.stop_node. Add a destructor to TestNode that cleans this up.
sdaftuar commented at 3:13 PM on April 6, 2018: member
I have been able to reproduce the issue of bitcoind processes living on after tests exit in a couple ways:
- #12375: running wallet_encryption.py on macos fails and leaves a bitcoind running
- the feature_dbcrash.py failure that should be fixed in #12902 does the same.
This PR causes all nodes to exit in both those situations.
MarcoFalke commented at 3:30 PM on April 6, 2018: member

utACK 9caecce6c7b00d3cc43eb220e91e9d1ac42251e8, makes sense
laanwj commented at 4:59 PM on April 6, 2018: member

Should this heed --noshutdown?
in test/functional/test_framework/test_node.py:91 in 9caecce6c7 outdated
```
  83 | @@ -84,6 +84,13 @@ def __init__(self, i, datadir, rpchost, timewait, binary, stderr, mocktime, cove
  84 |  
  85 |          self.p2ps = []
  86 |  
  87 | +    def __del__(self):
  88 | +        # Ensure that we don't leave any bitcoind processes lying around after
  89 | +        # the test ends
  90 | +        if self.process:
  91 | +            self.log.info("Cleaning up leftover process")
```
MarcoFalke commented at 5:04 PM on April 6, 2018:

Looks like the log is flushed and closed when the tests are done, so wouldn't work at this point.

sdaftuar commented at 6:30 PM on April 6, 2018:

Odd, I was able to see this logging work when I tested locally. Will remove.

jnewbery commented at 8:08 PM on April 6, 2018:

Yes, it appears that logging can work after calling logging.shutdown(), but the documentation states that no calls to logging should be made after shutdown: https://docs.python.org/3/library/logging.html#logging.shutdown
sdaftuar commented at 6:31 PM on April 6, 2018: member

Should this heed --noshutdown? @laanwj Oops, yes of course it should, will fix.
sdaftuar force-pushed on Apr 6, 2018
jnewbery commented at 8:09 PM on April 6, 2018: member

Seems reasonable. I don't think this can do any harm.

utACK ccc9031c69e67c336362b197c7decf14af98a877
fanquake added the label Tests on Apr 6, 2018
MarcoFalke commented at 2:40 PM on April 7, 2018: member

Might have to set self.process to None in feature_help.py?
promag commented at 1:58 PM on April 8, 2018: member

Tested ACK f1d91ef.

Using the instructions in #12902 (comment) with this change kills all test processes.

Code looks good, please squash.
jonasschnelli commented at 2:00 PM on April 8, 2018: contributor

utACK f1d91ef5a2e91c07060b39f0285a733ecf6831b3
[qa] Ensure bitcoind processes are cleaned up when tests end e36a0c0852
sdaftuar force-pushed on Apr 8, 2018
sdaftuar commented at 2:48 PM on April 8, 2018: member

Squashed f1d91ef -> e36a0c08529bccc695ec71a7ec1df89367cc1628
promag commented at 3:24 PM on April 8, 2018: member

Tested e36a0c0.
laanwj commented at 3:40 PM on April 8, 2018: member

utACK e36a0c08529bccc695ec71a7ec1df89367cc1628
laanwj merged this on Apr 8, 2018
laanwj closed this on Apr 8, 2018
laanwj referenced this in commit 15c3bb4268 on Apr 8, 2018
MarcoFalke commented at 5:23 PM on April 8, 2018: member

Post-merge tested ACK on Windows e36a0c0
MarcoFalke referenced this in commit 0752146e4d on Apr 20, 2018
MarcoFalke referenced this in commit 6c26df06ad on Apr 20, 2018
HashUnlimited referenced this in commit 9e629dc009 on May 13, 2018
laanwj referenced this in commit 531a0337ca on Jun 11, 2018
ccebrecos referenced this in commit a272cb997b on Sep 14, 2018
schancel referenced this in commit f5873a89b6 on Jun 9, 2019
jtoomim referenced this in commit 618005af73 on Jun 29, 2019
jonspock referenced this in commit 1ee762d9a4 on Jul 4, 2019
jonspock referenced this in commit 2c1056d027 on Jul 4, 2019
proteanx referenced this in commit 4d52f0671d on Jul 5, 2019
jonspock referenced this in commit 0b4a16fa7a on Jul 9, 2019
PastaPastaPasta referenced this in commit 003235b7cc on Mar 14, 2020
PastaPastaPasta referenced this in commit 6653068a03 on Mar 19, 2020
PastaPastaPasta referenced this in commit 187b2e6f8d on Mar 21, 2020
PastaPastaPasta referenced this in commit 7afc672d80 on Mar 24, 2020
ckti referenced this in commit 29f9c85d75 on Mar 28, 2021
UdjinM6 referenced this in commit 8d22516df1 on Jun 19, 2021
UdjinM6 referenced this in commit ad11ad8ec5 on Jun 24, 2021
UdjinM6 referenced this in commit 4fb22e4c31 on Jun 26, 2021
UdjinM6 referenced this in commit b02642d53f on Jun 26, 2021
UdjinM6 referenced this in commit 69fd5dee3d on Jun 26, 2021
UdjinM6 referenced this in commit 05dd5c7079 on Jun 28, 2021
gades referenced this in commit 82dab91071 on Jun 30, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#13384 qa: Remove polling loop from test_runner #13421 qa: Remove portseed_offset from test runner