Dji Tello Halt does not terminate all the related goroutines and may wait forever when it is called multiple times

[st-user]

(parent commit 27c76b4585807e829965ec6a4551cadc28f430fe)

I tried Dji Tello Driver and found the following two problem related to Halt method.

If these are issues that need to be fixed, I’d like to make a pull request.

Problems

1. Halt stops only one of the three infinite loops.

The Driver runs the three infinite loops(*). However Halt stops only one of the three because:

• case <- d.doneCh: break… is defined in two of the three.
• Halt calls d.doneCh <- struct{}{} only once.

(*)

• https://github.com/hybridgroup/gobot/blob/release/platforms/dji/tello/driver.go#L289
• https://github.com/hybridgroup/gobot/blob/release/platforms/dji/tello/driver.go#L308
• https://github.com/hybridgroup/gobot/blob/release/platforms/dji/tello/driver.go#L949

So, only one of the three infinite loops can read d.doneCh and stop.

2. Halt may wait forever when it is called multiple times.

Simply because Halt is blocked by d.doneCh <- struct{}{}, if we call Halt after the driver has been halted, It blocked forever.

My test case and bug fixes

Test case

The test cases below simulate the problems.
These two test methods don't stop.
(st-user@1ffc138)

// Only output one line ('Done routine x.')
func TestHaltShouldTerminateAllTheRelatedGoroutines(t *testing.T) {
	d := NewDriver("8888")
	d.cmdConn = &WriteCloserDoNothing{}

	var wg sync.WaitGroup
	wg.Add(3)
	go func() {
		<-d.doneCh
		wg.Done()
		fmt.Println("Done routine 1.")
	}()
	go func() {
		<-d.doneCh
		wg.Done()
		fmt.Println("Done routine 2.")
	}()
	go func() {
		<-d.doneCh
		wg.Done()
		fmt.Println("Done routine 3.")
	}()

	d.Halt()
	wg.Wait()
}

func TestHaltNotWaitForeverWhenCalledMultipleTimes(t *testing.T) {
	d := NewDriver("8888")
	d.cmdConn = &WriteCloserDoNothing{}

	d.Halt()
	d.Halt()
	d.Halt()
}

Try fixing

I tried fixing the problems at the commit(st-user@8c5ac6f) by:

• Defining case <- d.doneCh: break… in every infinite loops.
• Calling d.doneCh <- struct{}{} as many times as the running infinite loops.

[joeyberkovitz]

On this topic, NewDriverWithIP doesn't initialize the doneCh (https://github.com/hybridgroup/gobot/blob/dev/platforms/dji/tello/driver.go#L238), so Halt is guaranteed to hang due to writing to a nil channel. I can submit a PR if needed to fix that function as well, although it's a trivial change

[gen2thomas]

Hi @joeyberkovitz thanks for pointing this out. A PR would be great.

Thanks!