go-ping / ping

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

Feat: `Pinger`'s `RunWithContext` method #185

Closed qdm12 closed 1 year ago

qdm12 commented 2 years ago
qdm12 commented 2 years ago

@SuperQ @CHTJonas I tried removing the .SetPrivileged(true) in https://github.com/go-ping/ping/pull/185/commits/5b70679101c4bc27ebe9fde4635ac342f1e98c6e

but the CI complains ping_test.go:803: expected <nil> but got socket: permission denied.

I believe you need to set as root sysctl -w net.ipv4.ping_group_range="0 2147483647" to fix that, so back to square one right? 🤔 Can we have root access anywhere on the CI perhaps before running the tests so we can run that command?

Alternatively, I can put that test in a separate test file with a build tag like:

//go:build integration
// +build integration

So it would only run with go test -tags integration and not in the CI, but that's risky business too 😥

alexflint commented 2 years ago

Hey all - I came looking for exactly the functionality in this PR. Any update on this?

qdm12 commented 2 years ago

@alexflint I guess you could just copy pasta that method as a function. That's what I did back then here: https://github.com/qdm12/gluetun/blob/7d824a51798f3e635e116dfece6056fd2ad4a2c6/internal/healthcheck/health.go#L59

tarndt commented 2 years ago

Here is an alternate solution I use to get this functionality:

package netutil

import (
    "context"

    "github.com/go-ping/ping"
)

type Pinger struct {
    *ping.Pinger

    ctx       context.Context
    ctxCancel context.CancelFunc
}

func NewPinger(addr string) (*Pinger, error) {
    pinger, err := ping.NewPinger(addr)
    if err != nil {
        return nil, err
    }

    return &Pinger{Pinger: pinger}, nil
}

func (p *Pinger) Run(ctx context.Context) error {
    if err := ctx.Err(); err != nil {
        return err
    }

    p.ctx, p.ctxCancel = context.WithCancel(ctx)
    defer p.ctxCancel()

    go func() {
        <-ctx.Done()
        p.Pinger.Stop()
    }()

    return p.Pinger.Run()
}

func (p *Pinger) Stop() {
    p.ctxCancel()
}
qdm12 commented 2 years ago

I think we should close this, and leave it to the user to implement context aware methods. What do you think?

SuperQ commented 1 year ago

See https://github.com/go-ping/ping/pull/226