Add a delay when testing tree monitor updates #1187
Conversation
|
@superstar54 can you please review this? 🙏 |
tests/test_status.py
Outdated
|
|
||
| def delayed_add_branches(): | ||
| self.tree.trunk._add_branches_recursive() | ||
| time.sleep(0.5) |
There was a problem hiding this comment.
I feel that the time delay after the _add_branches_recursive() will not work as expected. I think it should inside _add_branches_recursive, but I am not sure. Could you explain a bit of detail on the race condition? Especially when and how the race condition happened, on which variable?
There was a problem hiding this comment.
When a WorkChainTreeNode needs to add branches (sub-workchains, calculations), it calls a wrapper method add_branches, which wraps the recursive branch adding method with the new adding_branches flag. This flag is meant to be observed by any other thread that tries to do the same. This happens when the user expands a branch for the first time. This triggers adding sub branches. If during this action the monitor thread runs an update, since the branch is expanded, the update will also trigger adding branches. But now the monitor thread should see the flag is True and will skip this step. Before, without the flag in place, there was a chance that the monitor thread would process a child node before the main thread added that node's pk to the pks cache, which is checked to avoid duplication. This is how we ended up with duplicate branches. But the flag should prevent this. In the test, I replace this guarding wrapper method with a direct call to the recursive method, then sleep for a bit.
You may be correct. It is during the recursive call that a node may be handled twice without the presence of the flag. But here I sleep after the recursion is complete. This may not capture the event consistently. However, the test passes, which suggest we end up with a duplication as expected without the flag, and no duplication when the flag is there.
There was a problem hiding this comment.
However, the test passes
I remember you mentioned the test is not stable previously, some time passed while others time failed. I think the issue is still there in this PR.
|
@superstar54 this iteration should be closer to what you have in mind. The idea is to delay WHILE adding a child branch, right before adding its pk to the cache. This in principle allows the monitor thread to add the child BEFORE the pk is added, so the monitor thread will think it should be added. |
The test is time-dependent due to race conditions. This PR adds a small delay to ensure the main thread spends enough time adding branches to ensure that the monitoring thread will also attempt it.