papertrail / remote_syslog2

To install, see "Releases" tab. Self-contained daemon for reading local files and emitting remote syslog (without using local syslogd).
http://help.papertrailapp.com/
MIT License
637 stars 157 forks source link

Fixes goroutine leak for failed connections #213

Closed trevorlinton closed 6 years ago

trevorlinton commented 6 years ago

If a connection fails to establish a go routine is leaked from the Logger.writeLoop() running prior to returning logger,err on line 126/131.

You can test this by creating to go routines in a main function. One that just waits for 10 seconds and prints out the amount of go routines, and the other that proceeds to try and connect to an bad host/port over and over with a dial/connect timeout of 1.

Some psuedo code to demonstrate:

package main
import (
        "fmt"
        "github.com/papertrail/remote_syslog2/syslog"
        "time"
)
func main() {
    client : = ...
    host := ...
    t := time.NewTicker(time.Second * 10)
    for {
        syslog.Dial(client, "tcp", host, nil, time.Second*1, time.Second*2, 99990)
        fmt.Println("Go routines: ", runtime.NumGoroutine())
        <-t.C
        }
}
trevorlinton commented 6 years ago

Ah, that makes more sense, so if we get an error back it could be an error that's temporary and could be ignored or it could be a bad hostname or something that will never resolve, but there may not be any way of knowing between either.

Should it just be a safe policy to run logger.Close() if we receive an err back?

In practice what was happening is we'd have bad hostnames end up in the syslog end point, and when it failed we didn't call logger.Close() as we thought that the err being populated would insinuate the logger was nil like in golangs default net/dail.

snorecone commented 6 years ago

Yeah the code could definitely use some work in that respect, and you're right, there currently is not a way to tell if an error will be permanent or not. You could do as you describe and inspect the error and optionally close the logger.

trevorlinton commented 6 years ago

i'll chalk this up to user error on my part. Thanks for the explanations!