Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: pass dep name through params in callback #70

Closed
wants to merge 1 commit into from

Conversation

radiohertz
Copy link

fixes #69

although i think Go 1.22 fixes the behavior that causes this in the lang (https://go.dev/blog/go1.22)

@radiohertz
Copy link
Author

cc @jessepeterson

@JieAnthony
Copy link

@jessepeterson
Please check this PR, it is very important to me

@jessepeterson
Copy link
Member

I have seen this and its on my list. Apologies been a busy week! I'll hopefully get a look at it early next week. Thanks!

@jessepeterson
Copy link
Member

I've had a look-see at this and, indeed, there's a bug where we're launching goroutines from an anonymous callback function but trying to treat the name variable as scoped like a closure. Nice catch!

I think this could be fixed by keeping the closure, but passing the variables to the Go function to capture them. Each callback is linked to a given name, so we shouldn't need to pass in the name to the callback (it should already know what name it is).

How about this fix:

diff --git a/cmd/depsyncer/main.go b/cmd/depsyncer/main.go
index 1e70eeb..e5a7078 100644
--- a/cmd/depsyncer/main.go
+++ b/cmd/depsyncer/main.go
@@ -151,12 +151,12 @@ func main() {
                                }
                        }()
                        if webhook != nil {
-                               go func() {
+                               go func(name string) {
                                        err := webhook.CallWebhook(ctx, name, isFetch, resp)
                                        if err != nil {
                                                logger.Info("msg", "calling webhook", "err", err)
                                        }
-                               }()
+                               }(name)
                        }
                        return nil
                }

Does that work instead? If that doesn't work then I'd settle for this patch as is. Thanks!

@JieAnthony
Copy link

@jessepeterson
I'm very sorry, I don't know how to develop and debug GOLANG. This issue needs to be dealt with when you are free. Thank you for your great work.

@radiohertz
Copy link
Author

@jessepeterson updated the patch!

@jessepeterson
Copy link
Member

So, I was mistaken and @radiohertz was exactly correct in their first suggestion: this is the classic Go for-loop variable scoping foot gun. I was able to finally mock this up with two DEP names and fix it as:

diff --git a/cmd/depsyncer/main.go b/cmd/depsyncer/main.go
index 1e70eeb..6066c0c 100644
--- a/cmd/depsyncer/main.go
+++ b/cmd/depsyncer/main.go
@@ -127,6 +127,7 @@ func main() {
        var wg sync.WaitGroup
 
        for _, name := range flag.Args()[0:] {
+               name := name
 
                // create the assigner
                assignerOpts := []depsync.AssignerOption{

I can quickly commit that that. :)

jessepeterson added a commit that referenced this pull request Jul 9, 2024
@JieAnthony
Copy link

@jessepeterson
Can you release a new version for this PR?

@JieAnthony JieAnthony mentioned this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]depsyncer callback error dep_name
3 participants