go-ping / ping

ICMP Ping library for Go
MIT License
1.31k stars 344 forks source link

logger.Fatal() causes entire app to exit #208

Closed stanimirivanovde closed 1 year ago

stanimirivanovde commented 2 years ago

I'm using go-ping to discover the hosts in an entire subnet. But some of the hosts get "no route to host" UDP error which causes go-ping to do a logger.Fatal() which does an os.exit(). I'm using zap.SugarLogger() as a logger. I don't care about the error since I want to continue scanning through but this makes it impossible. Can we switch this to log.Error() instead?

        case r := <-recvCh:
            err := p.processPacket(r)
            if err != nil {
                // FIXME: this logs as FATAL but continues
                logger.Fatalf("processing received packet: %s", err)
            }

        case <-interval.C:
            if p.Count > 0 && p.PacketsSent >= p.Count {
                interval.Stop()
                continue
            }
            err := p.sendICMP(conn)
            if err != nil {
                // FIXME: this logs as FATAL but continues
                logger.Fatalf("sending packet: %s", err)
            }
        } 

As a library go-ping should return the error and let the client code deal with it if needed.

CHTJonas commented 2 years ago

Partial dup of #16, but I don't think we've had a report yet about Fatalf exiting the rest of someone's application, so I'll leave this open.

stanimirivanovde commented 2 years ago

From zap logger's repo: https://github.com/uber-go/zap/blob/d38507c4e0f79e79e89f06b6ef570e216921ace9/sugar.go#L181

// Fatalf uses fmt.Sprintf to log a templated message, then calls os.Exit.
func (s *SugaredLogger) Fatalf(template string, args ...interface{}) {
    s.log(FatalLevel, template, args, nil)
}

Then: https://github.com/uber-go/zap/blob/81879d1b480621999f3e3bc8d61bcf08a999298f/zapcore/entry.go#L167

    // WriteThenFatal causes a fatal os.Exit after Write.
    WriteThenFatal

And finally: https://github.com/uber-go/zap/blob/81879d1b480621999f3e3bc8d61bcf08a999298f/zapcore/entry.go#L233

    case WriteThenFatal:
        exit.Exit()
stanimirivanovde commented 2 years ago

If I ping a non-existing address on my LAN with Mac OS ping I get sporadic results:

ping 192.168.93.209
PING 192.168.93.209 (192.168.93.209): 56 data bytes
ping: sendto: Host is down
ping: sendto: Host is down
Request timeout for icmp_seq 0
ping: sendto: Host is down
Request timeout for icmp_seq 1
ping: sendto: Host is down
Request timeout for icmp_seq 2
ping: sendto: Host is down
Request timeout for icmp_seq 3
ping: sendto: Host is down

The difference is that when go-ping encounters ping: sendto: Host is down it calls logger.Fatalf() and it kills the entire app.

The actual behavior of logger.Fatalf() is completely driven by the underlying implementation. This is why it is probably the best to use logger.Errorf() instead.

stanimirivanovde commented 2 years ago

I've done extensive testing on Windows and Linux and the issue doesn't appear there. It has to be a UDP connection error associated with Mac OS X only. I deployed the application on another Mac last week and when UDP errors are encountered Fatalf() exits the entire application . I need this fixed otherwise this library stops my entire application when I deploy it on Mac OS. I'll submit an MR.

stanimirivanovde commented 2 years ago

https://github.com/go-ping/ping/pull/214