lang: ast: Fix data race in ExprSingleton

[purpleidea]

(Note: the race test is still failing, but this is my fault-- it seems I forgot a few locks, I will add those shortly...)

[gelisam]

Neat approach-- however, is this really what we want?

If we're running Graph from multiple places and the result is supposed to be identical, then wouldn't that mean it's the same graph, so it should really only be pulled in once?

If it's not identical for each caller, then the code is wrong.

We clearly have a different understanding of what a Graph() implementation is supposed to do. If I call Graph() twice, I expect twice as many Func nodes to be added to the Graph(). This way, if Map calls a function twice, each call will have its own Func nodes.

[purpleidea]

If we have one object obj being used by two callers, then we're flipping the contents of obj.singletonGraph back and forth... Isn't that an issue? Or more accurarely, the first of the two that happens to call Graph() has it's contents stored in obj.singletonGraph and obj.singletonExpr that the second one will now use instead of generating it's own from obj.Definition.Graph...

    if obj.singletonExpr == nil {
        g, f, err := obj.Definition.Graph(env)
        if err != nil {
            return nil, nil, err
        }
        obj.singletonGraph = g
        obj.singletonExpr = f
        return g, f, nil
    }

    return obj.singletonGraph, obj.singletonExpr, nil

Isn't that an issue?

[gelisam]

the first of the two that happens to call Graph() has it's contents stored in obj.singletonGraph and obj.singletonExpr that the second one will now use instead of generating it's own from obj.Definition.Graph...

Yes, that's the whole point of ExprSingleton. You want every use site of a variable to use the same Func, right? The obvious way to do that is to call Graph() on every definition site, and then to have every use site point to the Func generated at the definition site. But that doesn't work because some of the variables are builtins, and you don't want to instantiate the builtins unless they are used. Solution: one of the use sites, doesn't matter which one, generates the Func, and every other use site reuses that Func.

[gelisam]

It would be the first place we needed mutexes in the AST which feels unusual.

We need to store the data somewhere. We could also do it by giving Graph() a mutable map of all the singletons, so that we can lock that map instead of the ExprSingleton. Would that be better?

[gelisam]

If we had the Txn input to Graph() would this still be an issue without the mutex?

Yes, there would still be a race: if two use sites run Graph() at the same time and there is no lock, two Funcs will be added instead of one, regardless of whether the Func is added by returning a graph or via Txn.AddGraph().

[purpleidea]

Yes, that's the whole point of ExprSingleton.

Okay gotcha. I feel like I had this equivalent sort of behaviour previously before we implemented lambdas... But maybe not? Obviously out of my depth here.

Let's discuss on the weekend a few things, but for this patch, I can do the plumbing bits for the lock initialization stuff, it's just plumbing.

Thanks!

[gelisam]

I feel like I had this equivalent sort of behaviour previously before we implemented lambdas... But maybe not?

Indeed! Before lambdas, every Expr was tied to exactly one Func, thus making it impossible for Map to produce more than one Func for a single lambda Expr. After lambdas, no Expr was tied to exactly one Func, thus solving Map but introducing a new problem for variables. After monomorphic bind, only some Exprs are tied to exactly one Func: those which are wrapped in ExprSingleton. I decided to attach the functionality to ExprSingleton instead of StmtBind in order to make it easier to reuse the functionality elsewhere than StmtBind, and indeed, it turns out that builtin variables need this functionality even though they are not introduced by StmtBind.

[purpleidea]

Okay, fantastic. I've fixed the panics and merged this to master! I didn't plumb through all the Init(data) stuff, since I don't think it mattered for now.