goroutine leak - kind of...

[youngkin]

Hi there,

Thanks for putting together these exercises! They're giving me a way to learn some areas of Go I don't get a chance to use very often at work.

I have a question regarding this lesson. This question is a bit long, so thanks in advance for reading through it.

Like your solution, my solution for gathering user input used a goroutine. Also, as with your solution, my solution used a time.Timer to timeout. Finally, as with your solution, my solution "kind of sort of" leaks the goroutine that's monitoring stdin for user input. That goroutine might also write to a closed channel once the user does hit "Return".

I say it "kind of sort of" leaks this goroutine because the process will exit shortly after the timeout, so the leak in this case is very short-lived. But still... And if the parent goroutine closes the channel after a timeout the application will panic:

panic: send on closed channel

goroutine 25 [running]:
main.main.func1(0xc4200762a0)
	/Users/rich_youngkin/Software/repos/go/src/github.com/youngkin/gophercises/quiz/solution/main.go:38 +0x121
created by main.main
	/Users/rich_youngkin/Software/repos/go/src/github.com/youngkin/gophercises/quiz/solution/main.go:35 +0x5d2

To make this happen I closed the channel on what is now line 44 in your part 2 solution:

		select {
		case <-timer.C:
			fmt.Println()
        Added ======>	close(answerCh)    <======= this line
			break problemloop
		case answer := <-answerCh:
			if answer == p.a {
				correct++
			}
		}

This is all a bit messy. I've spent some time thinking about how to have the user input goroutine also timeout and exit without attempting to write to the potentially closed channel. But after issuing the read to stdin it seems like there's no nice way to timeout the actual waiting for input, or to check if a timeout occurred while waiting for user input. For example, this won't reliably work as the order in which the case conditions are evaluated is random:

	ans, _ := in.ReadString('\n')
	select {
	case c <- ans:
		return
	case <-ctx.Done():
		return
	}

I was wondering if you had any thoughts about how to accomplish this and ensure the goroutine eventually exits without attempting to write to the channel.

Cheers,
Rich

[joncalhoun]

Thanks for putting together these exercises! They're giving me a way to learn some areas of Go I don't get a chance to use very often at work.

Awesome! I'm glad you are enjoying them :)

I say it "kind of sort of" leaks this goroutine because the process will exit shortly after the timeout, so the leak in this case is very short-lived. But still...

In my opinion, context is very important. In this context, a single goroutine leak isn't problematic, and it is why things like the Tick function exist even though it spawns a goroutine that will never be recovered by the garbage collector.

I'm guessing at least 50% of your question here stems from curiousity and wanting to learn, which is a great thing, but just don't let this type of block you from progressing :)

But after issuing the read to stdin it seems like there's no nice way to timeout the actual waiting for input, or to check if a timeout occurred while waiting for user input. For example...

The bigger issue is that once you call something like fmt.Scanf or ReadString your goroutine is going to block there. It will not move on to anything else until it reads something, and with Stdin this doesn't happen until the user hits enter.

There are likely "proper" ways to resolve this where you use something like termbox and completely take control of the entire terminal, but that is way too much work for what you gain here. There may be other ways but I'm not familiar with how to achieve them.

Honestly, if I had to do something about it what I would likely do is just add a "press enter to continue..." at the end of my program and use that to read from stdin one last time. Eg:

func main() {
  // ... existing code

  // Move this outside the problemLoop
  answerCh := make(chan string)

problemloop:
  for i, p := range problems {
    fmt.Printf("Problem #%d: %s = ", i+1, p.q)
    go func() {
      fmt.Println("goroutine is starting...")
      var answer string
      fmt.Scanf("%s\n", &answer)
      answerCh <- answer
      fmt.Println("goroutine is done!")
    }()

    select {
    case <-timer.C:
      fmt.Println()
      break problemloop
    case answer := <-answerCh:
      if answer == p.a {
        correct++
      }
    }
  }

  fmt.Printf("You scored %d out of %d.\n", correct, len(problems))
  fmt.Println("Press enter to quit.")

  // our main function blocks here until the final goroutine reads and terminates.
  <-answerCh
}

It is a little hacky (and the fmt.Println statements are for demonstration only), but it would work I believe.

[rjpgt]

Hi,
As I had written to you before I was interested in the problem in which the timeout is per question rather than per quiz. Certainly the problem of the leaking goroutine is more relevant here because you do not want each input goroutine for each question to hang indefinitely waiting for input.

You had suggested that I somehow reuse the previous input goroutine and that is what I have tried to do. I have a variable prevAnswered, which will be true if the previous question was answered before timeout, that will decide whether a new input goroutine is created for the next question or whether the previous goroutine itself is going to be used.

Of course the last input goroutine will hang( which you have solved above ) but as you said earlier that is not much of a problem because the program in then ending anyway.

package main
 
import (
    "encoding/csv"
    "flag"
    "fmt"
    "math/rand"
    "os"
    "strings"
    "time"
)
 
type quizQuestion struct {
    question string
    answer   string
}
 
type quiz []quizQuestion
 
func main() {
    pinFileName := flag.String("csv", "problems.csv", "File of questions and answers where every line is of the form 'question,answer'")
    ptimeout := flag.Int("timeout", 5, "Question time limit")
    pshuffle := flag.Bool("shuffle", false, "Shuffle the questions")
    flag.Parse()
 
    inFile, err := os.Open(*pinFileName)
    if err != nil {
        panic(err)
    }
    defer inFile.Close()
    qz := readCsv(inFile)
    doQuiz(qz, *ptimeout, *pshuffle)
}
 
func readCsv(f *os.File) quiz {
    var q quiz
    r := csv.NewReader(f)
    lines, err := r.ReadAll()
    if err != nil {
        panic(err)
    }
 
    for _, record := range lines {
        question := strings.ToLower(strings.Trim(record[0], " "))
        answer := strings.ToLower(strings.Trim(record[1], " "))
        q = append(q, quizQuestion{question, answer})
    }
    return q
}
 
func doQuiz(qz quiz, timeout int, shuffle bool) {
    var correct int
    ansChan := make(chan string)
    fmt.Print("The quiz will start when your press ENTER")
    _, err := fmt.Scanf("\n")
    if err != nil {
        panic(err)
    }
    var indices []int
    if shuffle {
        rand.Seed(time.Now().UnixNano())
        indices = rand.Perm(len(qz))
    } else {
        for i := range qz {
            indices = append(indices, i)
        }
    }
    prevAnswered := true
    for _, index := range indices {
        qzQ := qz[index]
        fmt.Print(qzQ.question, " = ")
        timer := time.NewTimer(time.Duration(timeout) * time.Second)
        /*
        start a new input goroutine only if the previous one
        has returned otherwise use that itself and don't create
        a new input goroutine.
        */
        if prevAnswered {
            go func() {
                ans := ""
                _, err := fmt.Scanf("%s\n", &ans)
                if err != nil {
                    ans = ""
                }
                ansChan <- ans
            }()
        }
        select {
        case <-timer.C:
            prevAnswered = false
            fmt.Println()
            break
        case ans := <-ansChan:
            prevAnswered = true
            if strings.ToLower(strings.Trim(ans, "\n ")) == qzQ.answer {
                correct++
            }
            if !timer.Stop() {
                <-timer.C
            }
        }
    }
    fmt.Println()
    fmt.Printf("You answered %d questions out of a total of %d questions correctly.\n", correct, len(qz))
}

[youngkin]

Hi Jon,

Thanks for the thoughtful answer. My question was indeed rooted less in this specific exercise and more towards wondering if there was a way to timeout a read from stdout.

I do think this exercise provides a learning moment regarding goroutine leaks. Perhaps you could highlight this as part of the exercise.

Thanks again,
Rich

Cheers,
Rich