fix: Close leaking goroutines when conn == nil

[kskitek]

Problem

When the connection is not established (for example due to the wrong realm) Client.Close() leaves goroutines behind.

While testing, I have spawned 33 clients and closed 32 - this should leave me with just 2 goroutines but I have 34 instead.

Details

• signalflow.NewClient spawns two goroutines:
  • Client.run
  • wsConn.Run
  • as long as the connection is not established it loops, trying to establish the connection
  • in case of context.Cancelled (triggered by Close()) it just leaves
• Client.Execute is called by the user
  • this calls Client.sendMessage and waits in serializeAndWriteMessage on one of:
  • c.conn.OutgoingTextMessages() <- &outgoingMessage
  • return <-resultCh

Solution

Fix closing of the client

• wsConn.Run can:
  • drain the OutgoingTextMessages to unblock Client.serializeAndWriteMessage
  • push nil error to outgoingMessage.resultChan to unblock Client.serializeAndWriteMessage
  • close wsConn.outgoingMessage to unblock shutting down in wsConn.run

Refactor NewClient

I didn't take this path - random thoughts of other changes to fix the situation.

• do not spawn goroutines when initializing the client
• add possibility to initialize the client synchronously - let the user handle non retryable errors
• allow to execute the program only when there is a connection

Testing

The scenario can be tested manually by:

• providing invalid realm
• running client.ExecuteProgram
• calling client.Close

I have provided a test Test_Close_DoesNotLeakGoroutines that simulates this situation and compares how many goroutines are before and after the test.

[benkeith-splunk]

Thanks for fixing this @kskitek. I'm looking at this and considering the different approaches and seeing if there is a better way. If I don't find anything we'll merge this. Thanks.

[benkeith-splunk]

Hey @kskitek sorry for the delay. This client was for a while needing a refactoring so I went ahead and made https://github.com/signalfx/signalfx-go/pull/198 that will be a new v2 module. I incorporated your goroutine check and have completely reworked how shutdown happens to hopefully avoid any leaks and make the client generally more robust.

[kskitek]

@benkeith-splunk Thank you for the fix! I like the changes in the reworked client. It will probably take us some time but we will migrate to v2.