3

Been trying to use gonnel for some tunnels. Tried to open 2 tunnels, and then when they got closed, I noticed that the log package is saying that is trying to close the same tunnel twice. From looking at the code, it seems that (one of) the problems is this: enter image description here

I thought I would take advantage of staticcheck, golangci-lint or even go vet to have it highlight this problem, so that I can be more confident on a fix. However, when I run the tools, I get this output:

golangci-lint: enter image description here

go vet:

enter image description here

staticcheck:

enter image description here

Also tried running go vet with -loopclosure, but I still don't get any output. Shouldn't one of the tools say that the go func needs the iterator variable passed as param? If it helps, I'm running go 1.14.3 darwin/amd64, and also tried importing the code in goland, but I didn't get an inspection warning for that piece of code. I am getting a feeling that I might be doing something wrong, could you spot what it is?

Thanks!

4
  • Ignoring the linter warnings, one issue could be that c.Tunnel contains duplicates. I noticed that AddTunnel doesn't check for duplicate insertions. If there are duplicates, there could be a race on t.IsCreated. Since CloseTunnel and DisconnectAll don't use t.IsCreated in a thread-safe way, it's possible that two of the closure goroutines can be spawned for the same tunnel if it's in c.Tunnel twice. Commented Jun 15, 2020 at 23:21
  • What version of Go (and linters) are you using? The warning was not in some older ones. Commented Jun 16, 2020 at 0:38
  • using go 1.14.3, staticcheck 2020.1.4, and golangci-lint has version 1.27.0 built from fb74c2e on 2020-05-13T18:45:55Z Commented Jun 16, 2020 at 11:20
  • Shouldn't one of the tools say that the go func needs the iterator variable passed as param? this is opinion based and you should talk about that with the lib authors. Commented Jun 21, 2020 at 15:38

1 Answer 1

2
+150

Yes, correct. This problem is very common among Gophers (What happens with closures running as goroutines?)!

Go vet's issues:

  1. https://github.com/golang/go/issues/21412
  2. https://github.com/golang/go/issues/16520

So, it seems you don't see an output with go vet because the goroutine in within an if block. Could you try doing it without the if block? I think your issue is more relevant Issue 21412


Similarly for golangci/govet:

Quoting from the golangci/govet/rangeloop.go's code:

This file contains the code to check range loop variables bound inside function
literals that are deferred or launched in new goroutines. We only check
instances where the defer or go statement is the last statement in the loop
body, as otherwise, we would need whole-program analysis.

Also, my suggestion would be to check if tunnels are open and then only close it. It might happen that you duplicate entries, so then again there's a possibility of some issue. And do fix the closure running as a goroutine error, first.

So, here's an example where the if block is a differentiator on how go vet works.

  • With if block, go vet returns nothing
package main

import (
        "fmt"
        "sync"
)

func main() {
        fruits := []string{"orange", "apple"}
        var wg sync.WaitGroup
        for _, fruit := range fruits {
                if true {
                        wg.Add(1)
                        go func() {
                                fmt.Println(fruit)
                                wg.Done()
                        }()
                }
        }
        wg.Wait()
}
  • Without if block, go vet returns loop variable fruit captured by func literal
package main

import (
        "fmt"
        "sync"
)

func main() {
        fruits := []string{"orange", "apple"}
        var wg sync.WaitGroup
        for _, fruit := range fruits {
                wg.Add(1)
                go func() {
                        fmt.Println(fruit)
                        wg.Done()
                }()
        }
        wg.Wait()
}
Sign up to request clarification or add additional context in comments.

2 Comments

@geo If this answer helped you, could you please accept it? Thanks!
hey @shmsr , sorry, didn't get a chance to try it! Will try it in the morning and then accept, thanks!

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.