Potential goroutine leak when using SSE subscriptions

[sybrandy]

The goroutine here listens for events on two channels. However, the goroutine has no way to know when it should stop. If I create an application that wants to close the marathon connection, this goroutine will continue to run in the background. It is highly suggested, and a best practice, that this goroutine be modified so that it can be stopped if the user wishes to close the connection to Marathon.

[timoreimann]

Hopefully the leak can be solved with the same pattern described in #198 (comment), which is to introduce a done channel that the receiver may use as a termination indicator.

[sybrandy]

I found a bug in the streams library you use that also causes leaks and reported it here. I patched the local copy I have an am testing it now, but I mention it because I wrapped the proposed close method in some code that should stop the goroutine. Also, I wanted you to be aware of this so that you could incorporate that fix into your code as well. Below is the snippet I created to fix the bug:

if len(r.listeners) == 0 {
    r.debugLog.Printf("registerSSESubscription(): stopping due to no listeners found.\n")
    r.subscribedToSSE = false
    stream.Close() // Call proposed in issue #6 in donovanhide/streams.
    break
}

From what I see in your code, this should work as it will shut down the goroutine when there are no listeners, but when one is added, it will be restarted. I'm testing this now and it looks like this is working, however I accidentally forgot a line of code so it still crashed after a random amount of time, so I can't say for certain. I'll let you know the results after I let this run a good long time.

[timoreimann]

@sybrandy thanks for the pointer to the upstream issue. I noticed that the eventsource maintainer left a few comments on the original PR that's presumably being revived now. Now that you are testing the patch, I wonder if you also intend to address those comments so that we can get a proper fix in?

Thanks!

[sybrandy]

I will provide feedback. The code has been running all weekend, so I need to review some logs to make sure everything is working as expected. Then I'll report my findings.

[iandyh]

Hi @timoreimann @sybrandy @gambol99 . I was going to ask similar questions until I found this thread.

There are two scenarios when the connection between go-marathon and Marathon Event Bus should be closed:

1. Applications want to close the connection for whatever reason (Currently there is no solution for the scenario as this PR stated. )
2. There are errors between the client and server, the connection should be closed and try with another Marathon node.

I purposed a PR for scenario 2 to the underlying eventsource library(basically by adding a timeout and then close the channels when timeout is reached) and the maintainer of that lib suggests to do the timeout at go-marathon side and send the signal to eventsource to close the channels. I am a little bit sceptical about the suggestion and wonder you guys' advice.

More discussions can be seen here: donovanhide/eventsource#24

Thanks for the reply!

[timoreimann]

@iandyh thanks for the pointer to the eventsource PR and discussion. I have left a comment there.