status-im / nimbus-eth2

Nim implementation of the Ethereum Beacon Chain
https://nimbus.guide
Other
551 stars 239 forks source link

Not flushing Taskpool when failure happens during spec test #5415

Open hcnam opened 1 year ago

hcnam commented 1 year ago

Taskpool is not flushed when a test fails. Therefore, the number of running threads increases when if fails during spec-test. Eventually, in extreme cases, this may result in resource exhaustion if a series of failing test cases exists.

For example, in fork choice spec test, it uses Taskpool like below.

let rng = HmacDrbgContext.new()
    taskpool = Taskpool.new()
var verifier = BatchVerifier.init(rng, taskpool)

However, if the test fails, taskpool does not terminate normally, so the number of running threads does not decrease. Therefore, it may be better to use a defer statement to terminate the thread when it fails.

let rng = HmacDrbgContext.new()
let taskpool = Taskpool.new()
defer:
    taskpool.shutdown()
var verifier = BatchVerifier.init(rng, taskpool)
mratsim commented 1 year ago

Are there tests that are supposed to fail though?

hcnam commented 1 year ago

While most tests would not fail, some tests may fail during development. (Otherwise, we wouldn't need the spec test in the first place!) What we report will make your spec test more robust when it handles any (rare though) failed tests.