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

daemon: Adds SdNotifyBarrier #343

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

rsms
Copy link

@rsms rsms commented Oct 2, 2020

This adds daemon.SdNotifyBarrier including a unit test. It is modeled after systemd's sd_notify_barrier.

The barrier function is useful when a signalling process wants to wait for systemd to process previously-sent messages.

  • I certify that I recognize and agree to the Developer Certificate of Origin of this project.
  • Tested on go 1.15.2-linux-amd64 and 1.15.2-linux-386
  • go fmt has been applied to the source code as that seems to be the case with other source files in this project.

Thanks for a useful package!

@rsms
Copy link
Author

rsms commented Oct 3, 2020

It appears an unrelated, unmodified test failed from timeout according to Travis:

...
--- FAIL: TestPropertiesSubscription (10.01s)
1721    subscription_test.go:188: Reached timeout
...

daemon/sdnotifybarrier.go Outdated Show resolved Hide resolved
//
// If `unsetEnvironment` is true, the environment variable `NOTIFY_SOCKET`
// will be unconditionally unset.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious blank line.

This feature has been added quite recently in systemd v246, so it would be good to mention the minimum requirement here.

Copy link
Author

Choose a reason for hiding this comment

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

Added a note about the systemd version.
The extra empty line at the end is a convention used by the go source code itself when a comment has multiple paragraphs. Since gofmt has no opinion on this it falls on you I guess, so I removed it :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I guess I'm up to some learning today. Do you have examples/references of this? Does some godoc renderer rely on this?

daemon/sdnotifybarrier.go Outdated Show resolved Hide resolved
@lucab
Copy link
Contributor

lucab commented Oct 5, 2020

@rsms thanks for the patch and the accompanying test! I've left a few comments in review, the only major one about using a context for timing out. The CI sometimes flakes a bit, in which case you can force-push to re-trigger it.

go func() {
select {
case <-ctx.Done():
pipe_r.SetReadDeadline(time.Now())
Copy link
Author

Choose a reason for hiding this comment

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

Note: pipe_r.Close() would work here too but setting an immediate read deadline has the upside of returning a descriptive error message: i/o timeout


func TestSdNotifyBarrier(t *testing.T) {

testDir, e := ioutil.TempDir("/tmp/", "test-")
Copy link
Author

Choose a reason for hiding this comment

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

Note: I used this since it's what I saw used in TestSdNotify and assumed there was a good reason (like a CI limitation.) If that's not the case, the testing package provides a nice TempDir() function that might be better for these tests (and more portable.) https://golang.org/pkg/testing/#T.TempDir

@rsms rsms requested a review from lucab December 17, 2020 02:09
@lucab
Copy link
Contributor

lucab commented Dec 17, 2020

@rsms can you please rebase on latest master? CI moved to github actions in the meanwhile.

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

Successfully merging this pull request may close these issues.

2 participants