Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Feb 12, 2021

Code is from ec135ee (#545), and while it was not wrong, the wrapping select isn't necessary for blocking on a single channel read. I only noticed because I'm working through a SIGQUIT stack trace which included:

goroutine 53 [select (no cases), 814 minutes]:
github.com/openshift/machine-config-operator/pkg/daemon.(*Daemon).runLoginMonitor(0xc000582a80, 0x0, 0xc000538480)
  /go/src/github.com/openshift/machine-config-operator/pkg/daemon/daemon.go:689 +0x30f
created by github.com/openshift/machine-config-operator/pkg/daemon.(*Daemon).ClusterConnect
  /go/src/github.com/openshift/machine-config-operator/pkg/daemon/daemon.go:299 +0x578

So with this commit, we drop the select and get straight into the blocking channel read.

Code is from ec135ee (daemon: Exec journalctl rather than using
library, 2019-03-15, openshift#545), and while it was not wrong, the wrapping
'select' isn't necessary for blocking on a single channel read.  I
only noticed because I'm working through a SIGQUIT stack trace which
included:

  goroutine 53 [select (no cases), 814 minutes]:
  github.com/openshift/machine-config-operator/pkg/daemon.(*Daemon).runLoginMonitor(0xc000582a80, 0x0, 0xc000538480)
    /go/src/github.com/openshift/machine-config-operator/pkg/daemon/daemon.go:689 +0x30f
  created by github.com/openshift/machine-config-operator/pkg/daemon.(*Daemon).ClusterConnect
    /go/src/github.com/openshift/machine-config-operator/pkg/daemon/daemon.go:299 +0x578

So with this commit, we drop the 'select' and get straight into the
blocking channel read.
@wking wking force-pushed the drop-useless-select branch from 075f880 to 2af50c1 Compare February 12, 2021 00:29
Copy link

@darkmuggle darkmuggle left a comment

Choose a reason for hiding this comment

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

Indeed, the select is superfluous.
/LGTM

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: darkmuggle, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2021
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit d434c55 into openshift:master Feb 19, 2021
@wking wking deleted the drop-useless-select branch March 20, 2021 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants