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

Add context based Run API #29

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

TheRushingWookie
Copy link
Contributor

@TheRushingWookie TheRushingWookie commented Apr 3, 2023

Description

This adds a context based API. This is useful for code which has an existing context and wants to rely on that instead of using a timeout or a count of packets to stop the pinger. When the context is canceled, the ping object is stopped.

I've also added a mutex to the tests as there is a data race detected in testPacketConnOK
See

make test                                                                                                
>> running all tests
GO111MODULE= go test -race -cover  ./...
==================
WARNING: DATA RACE
Write at 0x00c00018f7a8 by goroutine 17:
  github.com/prometheus-community/pro-bing.(*testPacketConnOK).WriteTo()
      /Users/qjarrell/repos/pro-bing/ping_test.go:720 +0x75
  github.com/prometheus-community/pro-bing.(*Pinger).sendICMP()
      /Users/qjarrell/repos/pro-bing/ping.go:752 +0x92a
  github.com/prometheus-community/pro-bing.(*Pinger).runLoop()
      /Users/qjarrell/repos/pro-bing/ping.go:509 +0x4c4
  github.com/prometheus-community/pro-bing.(*Pinger).run.func3()
      /Users/qjarrell/repos/pro-bing/ping.go:463 +0xcf
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /Users/qjarrell/git/go/1.18.2/pkg/mod/golang.org/x/sync@v0.1.0/errgroup/errgroup.go:75 +0x86

Previous read at 0x00c00018f7a8 by goroutine 14:
  github.com/prometheus-community/pro-bing.(*testPacketConnOK).ReadFrom()
      /Users/qjarrell/repos/pro-bing/ping_test.go:731 +0x55
  github.com/prometheus-community/pro-bing.(*Pinger).recvICMP()
      /Users/qjarrell/repos/pro-bing/ping.go:605 +0x1cd
  github.com/prometheus-community/pro-bing.(*Pinger).run.func2()
      /Users/qjarrell/repos/pro-bing/ping.go:458 +0xcf
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /Users/qjarrell/git/go/1.18.2/pkg/mod/golang.org/x/sync@v0.1.0/errgroup/errgroup.go:75 +0x86
==================

Compatibility

This change is backwards compatible for the current API but brings in a new dependency, context, which was added in Go 1.7. If a user is on an older than 1.7 version then they will not be able to move to the new version of pro-bing without upgrading their go version.

Testing

Added two unit tests

  • First test runs a context with a timer which expires in 100 ms
  • Second test runs with a never ending context and tests that reaching the max count will also stop the pinger nicely

ping.go Outdated Show resolved Hide resolved
@SuperQ
Copy link
Contributor

SuperQ commented Apr 4, 2023

We only support the last 2 Go releases, so there's no need to worry about Go older than 1.18.

@TheRushingWookie TheRushingWookie force-pushed the add_context branch 6 times, most recently from cb1da95 to 8adffb1 Compare April 4, 2023 17:22
@TheRushingWookie
Copy link
Contributor Author

Fixed lint issues

ping.go Outdated Show resolved Hide resolved
Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor naming nit, otherwise LGTM.

Signed-off-by: TheRushingWookie <3181551+TheRushingWookie@users.noreply.github.com>
…tPacketConnOK's member variables

Signed-off-by: TheRushingWookie <3181551+TheRushingWookie@users.noreply.github.com>
@SuperQ SuperQ merged commit a17ba03 into prometheus-community:main Apr 11, 2023
2 checks passed
@TheRushingWookie TheRushingWookie deleted the add_context branch April 11, 2023 21:26
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

Successfully merging this pull request may close these issues.

None yet

3 participants