timonkrebs / MemoizR

Declarative Structured Concurrency for C#
Apache License 2.0
106 stars 3 forks source link

Potential cyclic dependencies - acyclic behavior is not enforced #23

Closed linkdotnet closed 10 months ago

linkdotnet commented 10 months ago

Hey @timonkrebs,

awesome work on this one. I played around a bit and found a potential pitfall. You can create cyclic dependencies like this:

var factory = new MemoFactory();
MemoizR<string> memoA = null;
MemoizR<string> memoB = null;

memoA = factory.CreateMemoizR<string>(async () => {
    Console.WriteLine("Evaluating memoA...");
    await Task.Delay(50);
    return "Result A + " + await memoB.Get(); 
});

memoB = factory.CreateMemoizR<string>(async () => {
    Console.WriteLine("Evaluating memoB...");
    await Task.Delay(100);
    return "Result B + " + await memoA.Get();
});

Console.WriteLine(await memoA.Get());

This will produce:

Evaluating memoA...
Evaluating memoB...
Evaluating memoA...
Evaluating memoB...
Evaluating memoA...
Evaluating memoB...
...

Depending on the graph, a DFS might be pretty expensive and not something you want to have in production - so it might be a consideration to expose an API to check for that and shift the responsibility to the user. Alternatively, code-docs might be totally enough here.

timonkrebs commented 10 months ago

You are right, this is not as save as it could be. I did not prioritize it as it is only v0.1.0 and I thought early adopters would probably be able to handle this.

I think for the moment I will introduce the same mechanism as solidjs uses. By running the provided function immediately this will lead to a runtime error.

But I think I will make it configurable, so that you can opt out of this for perf reasons.

timonkrebs commented 10 months ago

@linkdotnet What do you think about #25? It would prevent noobies from running into these stupid bugs by giving instant feedback where the problem was introduced. But also enable users to get rid of the overhead. And It would make it easy to locate where the problem was introduced by running it in SaveMode.

But I am not really happy with this solution. I probably will change the way I get rid of these problems. Maybe a better option is to solve it with an analyzer.

linkdotnet commented 10 months ago

Yes, that would go in a similar direction. Opting-In into a save mode might be ever the better choice, given the fact, that usually, beginners don't have very advanced use-cases (and therefore graphs) that need guardrails.

I do think for now that is absolutely fine to keep it as it is. As you mentioned it is v0.1 and my example is pretty artificial in the first place.