lang: funcs: Don't race when building an initial graph
I noticed a very intermittent test failure where interpret would end up running, but *fail* because a value wasn't present. This should never happen, because the function engine is designed to only call interpret when there has been at least one value produced for every node in the AST. So what is the bug that would produce: interpret error: could not interpret: func value does not yet exist About 20 minutes ago while I was getting to bed, it occurred to me where to look! Out of bed and to the laptop, and after briefly reminding myself of the code, I think I've found the issue. What I think was happening, was that an AST node would produce a value, and send a message on the aggregate channel. This channel is monitored, and every time it receives a message, it checks to ensure that all the values now exist before producing a message for interpret to run. However, this AST node was not the final one to be produced, but before the message was read by the aggregate channel, the last remaining AST node ran and set it's "loaded" state to `true`, but *before* its value was made available for the aggregate channel to read. That channel then occasionally won the race and tried to access a value before it existed, thus causing out intermittent bug. At least I think that's what was going on. Hopefully this patch fixes this, if not, then there's another bug hiding too! And of course, this entire function engine could do with some proper analysis from someone familiar with glitches, back pressure, and FRP parallelism. One particular note was that I used my brain, not some fancy debugging tool to find this. Maybe skilled debuggers can fork lift their tools onto this type of problem, but I haven't those skills! ¯\_(ツ)_/¯
This commit is contained in:
@@ -435,7 +435,8 @@ func (obj *Engine) Run() error {
|
||||
cached, exists := obj.table[vertex]
|
||||
obj.mutex.RUnlock()
|
||||
if !exists { // first value received
|
||||
node.loaded = true
|
||||
// RACE: do this AFTER value is present!
|
||||
//node.loaded = true // not yet please
|
||||
obj.Logf("func `%s` started", node)
|
||||
} else if value.Cmp(cached) == nil {
|
||||
// skip if new value is same as previous
|
||||
@@ -452,6 +453,7 @@ func (obj *Engine) Run() error {
|
||||
if err := node.Expr.SetValue(value); err != nil {
|
||||
panic(fmt.Sprintf("could not set value for `%s`: %+v", node, err))
|
||||
}
|
||||
node.loaded = true // set *after* value is in :)
|
||||
obj.mutex.Unlock()
|
||||
obj.Logf("func `%s` changed", node)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user