Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Goroutine leaks? #6

Open
technosophos opened this issue Jun 25, 2014 · 4 comments
Open

Goroutine leaks? #6

technosophos opened this issue Jun 25, 2014 · 4 comments

Comments

@technosophos
Copy link
Contributor

The documentation suggests doing this:

go pubInstance.Publish(<pubnub channel>, <message to publish>, callbackChannel, errorChannel)
go ParseResponse(callbackChannel)
go ParseErrorResponse(errorChannel) 

It appears that Publish() will only send messages on one of the two channels. And (like a good Go receiver) never closes the channels that it receives.

Won't this lead to leaking at least one goroutine for every single Publish?

See, at least one of those two goroutines will block on <-myChannel, which means that goroutine will never be freed.

Wouldn't it make more sense to use a select{} on those two channels and react to whichever one returns first? And probably having a time.Timer to timeout long requests would be a good idea, too, right?

@geremyCohen
Copy link
Contributor

thanks for the feedback @technosophos! we're taking a look... please standby.

@technosophos
Copy link
Contributor Author

I used runtime.NumGoroutine() to monitor the goroutines. I'm now testing an implementation that looks like this:

func main() {
        // Other setup stuff
    go pm.Publish(channel, message, callbackChannel, errorChannel)
    go handleResult(callbackChannel, errorChannel)
}

func handleResult(good, bad chan []byte) {
    timeout := time.NewTimer(time.Second * 10)
    for {
        select {
        case success := <-good:
            // handle success
            return
        case failure := <-bad:
            // handle error
            return
        case <-timeout.C:
            // timeout
            return
        }
    }
}

This has stopped the goroutines from stacking up. So far the timeout has never triggered, so I am not sure if that part is working.

Based on my testing, I've dropped from hundreds of lingering goroutines to just a few.

@geremyCohen
Copy link
Contributor

@technosophos thanks for the contribution! We'll get this into our next release, within a week or so.

@crimsonred
Copy link
Contributor

@technosophos , the read me has been modified as per your suggestion. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants