status-im / nim-chronos

Chronos - An efficient library for asynchronous programming
https://status-im.github.io/nim-chronos/docs/chronos
Apache License 2.0
353 stars 51 forks source link

Add an option to reset the trackers #316

Open lchenut opened 2 years ago

cheatfate commented 2 years ago

can i get reasons for this option?

lchenut commented 1 year ago

This is mainly for libp2p tests (but there may be other applications). When a tracker fails at the beginning of the tests, all other tests checking that tracker fail as well even if there's no problem. This is just an option to reset and improve readability.

arnetheduck commented 1 year ago

Rather than just resetting the trackers, it seems it would be useful in general to reset all global state that chronos creates - or at least as much as possible - that includes the global dispatcher etc - resetting just the trackers will merely lead to an inconsistency between the various globals that are spread out throughout the library (and indeed make testing harder)

cheatfate commented 1 year ago

If this tracker check fails it means that you are leaking or do not properly cleanup in test. All the trackers working like that, i dont see any reasons to fix chronos just because libp2p tests could not properly call close procedure.

Menduist commented 1 year ago

We know that the tracker is leaking, the point is to reset the state before other tests, otherwise all the following tests will fail.

Ideally we would indeed reset all the global states (open sockets etc), but that requires tracking everything in chronos to be able to reset everything

lchenut commented 1 year ago

I don't know where I said I wanted to fix chronos, I said that I wanted to improve readability when a tracker shows some leaks. Currently we have logs like this one that are hard to read (and wrong because there's only one test that fails).

This is simply a QoL feature.

cheatfate commented 1 year ago

We know that the tracker is leaking, the point is to reset the state before other tests, otherwise all the following tests will fail.

How tracker could leak? It just counters. From my point of view if you want to run group of tests you can bundle it into suite and run initialization and clean up in suite block.

arnetheduck commented 1 year ago

clean up

this is what reset in this PR is for: so that each test has a fresh counter value - otherwise, correct tests are shown as failing because a previous test failed.

cheatfate commented 1 year ago

Also i think you don't understand that if you reset counters it doesn't matter that some loops which are pending and running are not running anymore. Counters got decreased when this loops exiting. So even in tests if you just reset counters you can get pretty weird behavior when some of the loops which are still running could produce some undefined behavior for your tests.

arnetheduck commented 1 year ago

improve readability when a tracker

thinking a bit more about this PR, without resetting the dispatcher I think it makes little sense to reset the tracker alone - ie the only way to get a "clean slate" for every test would be to run each test in a separate process (which implicitly will reset the tracker) - such functionality could quite easily be added to unittest2 or somewhere.

Menduist commented 1 year ago

Also see https://github.com/status-im/nim-chronos/pull/326, which opens the question "should closing the dispatcher close the associated FDs" (which could have been moved elsewhere)