Add running state to spinner manager, allow running Stop multiple times

[KimNorgaard]

fix: add checks to Stop() to make it noop if the spinner manager is already stopped

I was running Stop() in a cleanup function and it had already been called in a defer somewhere else. This caused the program to be stuck because it could no longer communicate to the render() function through the channel. This change adds a running attribute used to determine if the spinner manager is running and return early from Stop() if it isn't.

feat: add Running() method to determine the running state of the spinner

It's just for convenience.

[chelnak]

Good catch, I can see why this would be useful. Thank you 🙂

[chelnak]

The CI run seems to be complaining but it doesn't look like it's related to your PR.

I'll investigate.

[chelnak]

👋 CI is fixed and I rebased your PR so that it could run through the tests.

It looks like the change introduces a data race. Fortunately it isn't hard to resolve.

I think we can move sm.running = false in to the Stop function rather than having it set when we see a notification in render.

I guess as an extra defence, it could be moved right after the lock here.. but that didn't seem to make much difference in my tests.

func (sm *spinnerManager) Stop() {
	if !sm.running {
		return
	}
	sm.done <- true
	sm.ticks.Stop()
	sm.running = false <--- Here

	// Persist the final frame for each spinner.
	sm.mutex.Lock()
	defer sm.mutex.Unlock()
	for _, s := range sm.spinners {
		tput.ClearLine(sm.writer)
		s.Print(sm.writer, sm.chars[sm.frame])
	}
}

I was running the tests with this command and it was happy.

go test -timeout 30s -run ^TestManagerStopTwice$ github.com/chelnak/ysmrr -race -count=10000

Once this is resolved, I'm happy to get this merged.

[KimNorgaard]

Good catch. I hadn't thought of the concurrency issue this could cause. I've made the change and tested.

On another note, make lint complains a little. Nothing big but thought you might want to know.

[chelnak]

Awesome thank you.

The linter issues should be resolved on main now 🤞.