This repository was archived by the owner on Sep 10, 2025. It is now read-only.
Repository navigation
This repository was archived by the owner on Sep 10, 2025. It is now read-only.
Circular dependency - strange result #82
Description
This came from more of a "I wonder what would happen if..." thought.
Given the following circular dependencies:
log {
name => "log1"
message => "one"
require => "log2"
}
log {
name => "log2"
message => "two"
require => "log3"
}
log {
name => "log3"
message => "three"
require => "log1"
}
The following output is given:
2022/01/09 18:42:06 [USER] two
2022/01/09 18:42:06 [USER] one
2022/01/09 18:42:06 [USER] two
2022/01/09 18:42:06 [USER] three
Now I'm not sure what I was expecting necessarily, possibly a never ending loop, possibly nothing at all, but certainly not that.
Activity
skx commentedon Jan 9, 2022
I could almost understand what is happening here to result in
two,one,two:The surprise is almost that we don't see:
I'm going to guess there's a bug in the handling of the "We've seen this rule already" code. It's either running the dependency, and not updating the seen-state, or it's running the rule without checking that the seen-state was updated as the dependency was previously-executed.
I'll take a look tomorrow, unless you want to explore in advance.. As you say it's almost hard to know what should happen. But I'd guess myself the correct result is:
three
two
one
rule one launches
so "ONE"
At that point the loop is broken because all the rules should be reported as "Already run".
I'm further guessing, we don't recurse for dependencies.
So if "one requires two" we'll handle that. But if "one requires two, which requires three", we'll only treat that as the first case.
skx commentedon Jan 9, 2022
Initial thoughts on a plan of attack:
Then separately add a "seen" bool to the rule-struct, and mark that. Drop the use of
e.seenI guess there's going to have to be an overhaul of the notifier too:
Seeing this bug-report almost makes me wonder how things are as useful as they are, and how it even works. Good find! I guess this is the sort of thing a proof of concept is supposed to identify, though I've already started to rely on it, so it's moved past that state..
alexwhitman commentedon Jan 9, 2022
Currently building a full dependency chain wouldn't be possible because there may be dependent rules in other files which aren't parsed until they're included.
I wonder if there's any merit in parsing all includes up front and then giving the rules within an implicit dependency on the file being included.
For illustration purposes only as the dependency would be implicit, not suggesting a change in syntax, it could work similar to
skx commentedon Jan 10, 2022
Agreed, inclusions complicate things such that we can't really have dependencies expressed across files.
I'll have to think about this some more. I think it is clear there are two distinct bugs here:
Those can be fixed, but the issue of handling dependencies properly, especially when included-files are used, is going to be hard without a significant overhaul. (It would have been easier/possible if the parser expanded things at parse-time - then all the rules would have been available at once, but I think moving those to run-time was the correct decision.)
Improve dependency handling
skx commentedon Jan 11, 2022
Opened a pull-request which significantly improves how dependencies are handled/executed.
However I've not explicitly detected cycles, nor made changes with regard to include-files.
With that code running the example code works as "expected":
alexwhitman commentedon Jan 11, 2022
That certainly improves things. I'll try out some more things and see if I can break it again. 😄
skx commentedon Jan 11, 2022
Good luck!
I'll merge, but leave this open. I guess this, and #74, are kinda bigger than a single example. It might be that another large overhaul is required to fix things up. (Or made the decision to keep things simple and leave the limitations alone!)
alexwhitman commentedon Jan 11, 2022
I've done some further testing and I'm not seeing anything else circular dependency related so this could probably be closed. I have found something else but I'll open a separate issue for it.