feat: add check rules to avoid cycle Ref

[sysulq]

When we change the examples/chat below, it's not hard to realise localCache and scaler are mutually dependent, which would lead to a stack overflow.

type localCache struct {
	weaver.Implements[LocalCache]
	mu    sync.Mutex
	test  weaver.Ref[ImageScaler]
	cache *lru.Cache[string, string]
	// TODO: Eviction policy.
}


type scaler struct {
	weaver.Implements[ImageScaler]
	weaver.Ref[LocalCache]
}

➜  chat git:(main) ✗ go run .                               
╭───────────────────────────────────────────────────╮
│ app        : chat                                 │
│ deployment : 1d053701-be60-4e2e-b170-d2d40d12423a │
╰───────────────────────────────────────────────────╯
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc02058c388 stack=[0xc02058c000, 0xc04058c000]
fatal error: stack overflow

runtime stack:
runtime.throw({0xd9b445?, 0x7f49e1f1fc70?})
        /home/liqi/.gvm/gos/go1.21.0/src/runtime/panic.go:1077 +0x5c fp=0x7f49e1f1fc20 sp=0x7f49e1f1fbf0 pc=0x43be7c
runtime.newstack()
        /home/liqi/.gvm/gos/go1.21.0/src/runtime/stack.go:1107 +0x5ac fp=0x7f49e1f1fdd0 sp=0x7f49e1f1fc20 pc=0x4563ec
runtime.morestack()
        /home/liqi/.gvm/gos/go1.21.0/src/runtime/asm_amd64.s:593 +0x8f fp=0x7f49e1f1fdd8 sp=0x7f49e1f1fdd0 pc=0x46daef

[mwhittaker]

Oh great find! Yeah, we should either detect and disallow cycles or allow cycles. We'll work on this.

[mwhittaker]

Oh great find! Yeah, we should either detect and disallow cycles or allow cycles. We'll work on this.

[renanbastos93]

@mwhittaker Can you help to understand more about for me fix that? I believe that we need to improve checker when to generate files *_gen.go to ensure this merge cycle, right?

[sysulq]

@renanbastos93 I fixed this problem by checking the cycle at runtime, details can be found in the link below, just for your information :-)

https://github.com/go-kod/kod/blob/main/registry.go#L168

[rgrandl]

Looks great!

Would you mind sending a PR to add this support in the main Service Weaver repo as well?

Thanks,

• Robert

On Wed, May 29, 2024 at 11:44 PM 大可 @.***> wrote:

@renanbastos93 https://github.com/renanbastos93 I fixed this problem by checking the cycle at runtime, details can be found in the link below, just for your information :-)

https://github.com/go-kod/kod/blob/main/registry.go#L168

— Reply to this email directly, view it on GitHub https://github.com/ServiceWeaver/weaver/issues/556#issuecomment-2138794335, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMZ5KTKHZ4XC5HNRAQHFKTZE3DDVAVCNFSM6AAAAAA32WPL5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZYG44TIMZTGU . You are receiving this because you are subscribed to this thread.Message ID: <ServiceWeaver/weaver/issues/556/2138794335 @.*** com>

[renanbastos93]

Awesome, @sysulq Would you like to open a PR to fix that?

[sysulq]

@rgrandl @renanbastos93 Sure, but I've been a bit busy lately, so I would get to it after some time. 🙂

[renanbastos93]

@sysulq I can help you maybe I could to open PR and you review it. What do you think?

[sysulq]

@renanbastos93 Definitely sure

[renanbastos93]

I've already opened my branch to implement it, but now I am understanding more details about the code generation and the registry component needed to code it. If there is any other important information, please share it with me.

[renanbastos93]

https://github.com/renanbastos93/weaver-issue-556 (this project simulate the bug)

[renanbastos93]

@mwhittaker I have been thinking about raising an error when there is a cyclic reference. Maybe we could change this approach to accept it. Have you ever thought about accepting cyclic references? I'll continue fixing it based on raising an error when avoiding cyclic references, but I'd like to bring this point up for reflection. I know that Golang can't import packages with cyclic references, but maybe we could think about that.