From 1ffc13879c54a4625591add72b7ccf74661e3771 Mon Sep 17 00:00:00 2001 From: st-user Date: Tue, 25 May 2021 09:51:57 +0900 Subject: [PATCH 1/4] Dji Tello Halt does not terminate all the related goroutines and may wait forever when it is called multiple times Add failing unit tests. --- platforms/dji/tello/driver_test.go | 45 ++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/platforms/dji/tello/driver_test.go b/platforms/dji/tello/driver_test.go index c8d25b2a8..18a584d6c 100644 --- a/platforms/dji/tello/driver_test.go +++ b/platforms/dji/tello/driver_test.go @@ -5,6 +5,7 @@ import ( "encoding/binary" "fmt" "io" + "sync" "testing" "time" @@ -14,6 +15,15 @@ import ( var _ gobot.Driver = (*Driver)(nil) +type WriteCloserDoNothing struct{} + +func (w *WriteCloserDoNothing) Write(p []byte) (n int, err error) { + return 0, nil +} +func (w *WriteCloserDoNothing) Close() error { + return nil +} + func TestTelloDriver(t *testing.T) { d := NewDriver("8888") @@ -134,3 +144,38 @@ func TestHandleResponse(t *testing.T) { }) } } + +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() +} From 8c5ac6f0acbdbdce4a8d71b5176ca3c41d601033 Mon Sep 17 00:00:00 2001 From: st-user Date: Tue, 25 May 2021 10:42:44 +0900 Subject: [PATCH 2/4] Dji Tello Halt does not terminate all the related goroutines and may wait forever when it is called multiple times Fix the issue. --- platforms/dji/tello/driver.go | 37 +++++++++++++++++++++++++----- platforms/dji/tello/driver_test.go | 14 +++++++++++ 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/platforms/dji/tello/driver.go b/platforms/dji/tello/driver.go index 77451b24c..180d02f8e 100644 --- a/platforms/dji/tello/driver.go +++ b/platforms/dji/tello/driver.go @@ -10,6 +10,7 @@ import ( "net" "strconv" "sync" + "sync/atomic" "time" "gobot.io/x/gobot" @@ -191,7 +192,8 @@ type Driver struct { throttle int bouncing bool gobot.Eventer - doneCh chan struct{} + doneCh chan struct{} + doneChReaderCount int32 } // NewDriver creates a driver for the Tello drone. Pass in the UDP port to use for the responses @@ -280,7 +282,10 @@ func (d *Driver) Start() error { d.cmdConn = cmdConn // handle responses + d.addDoneChReaderCount(1) go func() { + defer d.addDoneChReaderCount(-1) + d.On(d.Event(ConnectedEvent), func(interface{}) { d.SendDateTime() d.processVideo() @@ -304,13 +309,22 @@ func (d *Driver) Start() error { d.SendCommand(d.connectionString()) // send stick commands + d.addDoneChReaderCount(1) go func() { + defer d.addDoneChReaderCount(-1) + + stickCmdLoop: for { - err := d.SendStickCommand() - if err != nil { - fmt.Println("stick command error:", err) + select { + case <-d.doneCh: + break stickCmdLoop + default: + err := d.SendStickCommand() + if err != nil { + fmt.Println("stick command error:", err) + } + time.Sleep(20 * time.Millisecond) } - time.Sleep(20 * time.Millisecond) } }() @@ -321,7 +335,11 @@ func (d *Driver) Start() error { func (d *Driver) Halt() (err error) { // send a landing command when we disconnect, and give it 500ms to be received before we shutdown d.Land() - d.doneCh <- struct{}{} + readerCount := atomic.LoadInt32(&d.doneChReaderCount) + for i := 0; i < int(readerCount); i++ { + d.doneCh <- struct{}{} + } + time.Sleep(500 * time.Millisecond) d.cmdConn.Close() @@ -946,7 +964,10 @@ func (d *Driver) processVideo() error { return err } + d.addDoneChReaderCount(1) go func() { + defer d.addDoneChReaderCount(-1) + videoConnLoop: for { select { @@ -989,6 +1010,10 @@ func (d *Driver) connectionString() string { return res } +func (d *Driver) addDoneChReaderCount(delta int32) { + atomic.AddInt32(&d.doneChReaderCount, delta) +} + func (f *FlightData) AirSpeed() float64 { return math.Sqrt( math.Pow(float64(f.NorthSpeed), 2) + diff --git a/platforms/dji/tello/driver_test.go b/platforms/dji/tello/driver_test.go index 18a584d6c..5d8cfc345 100644 --- a/platforms/dji/tello/driver_test.go +++ b/platforms/dji/tello/driver_test.go @@ -151,17 +151,29 @@ func TestHaltShouldTerminateAllTheRelatedGoroutines(t *testing.T) { var wg sync.WaitGroup wg.Add(3) + + d.addDoneChReaderCount(1) go func() { + defer d.addDoneChReaderCount(-1) + <-d.doneCh wg.Done() fmt.Println("Done routine 1.") }() + + d.addDoneChReaderCount(1) go func() { + defer d.addDoneChReaderCount(-1) + <-d.doneCh wg.Done() fmt.Println("Done routine 2.") }() + + d.addDoneChReaderCount(1) go func() { + defer d.addDoneChReaderCount(-1) + <-d.doneCh wg.Done() fmt.Println("Done routine 3.") @@ -169,6 +181,8 @@ func TestHaltShouldTerminateAllTheRelatedGoroutines(t *testing.T) { d.Halt() wg.Wait() + + gobottest.Assert(t, d.doneChReaderCount, int32(0)) } func TestHaltNotWaitForeverWhenCalledMultipleTimes(t *testing.T) { From 7840b839f5b7998c14ba3de564a8871e783ded0a Mon Sep 17 00:00:00 2001 From: st-user Date: Sat, 12 Jun 2021 11:43:36 +0900 Subject: [PATCH 3/4] Dji Tello Halt does not terminate all the related goroutines and may wait forever when it is called multiple times Fix the test for Halt method so that it waits for all the related goroutines to complete. --- platforms/dji/tello/driver_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/platforms/dji/tello/driver_test.go b/platforms/dji/tello/driver_test.go index 5d8cfc345..d7ae751ae 100644 --- a/platforms/dji/tello/driver_test.go +++ b/platforms/dji/tello/driver_test.go @@ -154,27 +154,24 @@ func TestHaltShouldTerminateAllTheRelatedGoroutines(t *testing.T) { d.addDoneChReaderCount(1) go func() { - defer d.addDoneChReaderCount(-1) - <-d.doneCh + d.addDoneChReaderCount(-1) wg.Done() fmt.Println("Done routine 1.") }() d.addDoneChReaderCount(1) go func() { - defer d.addDoneChReaderCount(-1) - <-d.doneCh + d.addDoneChReaderCount(-1) wg.Done() fmt.Println("Done routine 2.") }() d.addDoneChReaderCount(1) go func() { - defer d.addDoneChReaderCount(-1) - <-d.doneCh + d.addDoneChReaderCount(-1) wg.Done() fmt.Println("Done routine 3.") }() From 46725e9d15b8b68223d09f41ad83542e0009f629 Mon Sep 17 00:00:00 2001 From: st-user Date: Sat, 12 Jun 2021 12:16:50 +0900 Subject: [PATCH 4/4] Dji Tello Halt does not terminate all the related goroutines and may wait forever when it is called multiple times Halt method waits forever when at least one of the goroutines is blocked by its Read method. To avoid this, I make Halt method close the connections before writing to doneCh. --- platforms/dji/tello/driver.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/platforms/dji/tello/driver.go b/platforms/dji/tello/driver.go index 180d02f8e..359f52187 100644 --- a/platforms/dji/tello/driver.go +++ b/platforms/dji/tello/driver.go @@ -334,18 +334,23 @@ func (d *Driver) Start() error { // Halt stops the driver. func (d *Driver) Halt() (err error) { // send a landing command when we disconnect, and give it 500ms to be received before we shutdown - d.Land() - readerCount := atomic.LoadInt32(&d.doneChReaderCount) - for i := 0; i < int(readerCount); i++ { - d.doneCh <- struct{}{} + if d.cmdConn != nil { + d.Land() } - time.Sleep(500 * time.Millisecond) - d.cmdConn.Close() + if d.cmdConn != nil { + d.cmdConn.Close() + } + if d.videoConn != nil { d.videoConn.Close() } + readerCount := atomic.LoadInt32(&d.doneChReaderCount) + for i := 0; i < int(readerCount); i++ { + d.doneCh <- struct{}{} + } + return }