Skip to content

Suggestion: Split tunnel.Client.Run into two methods #14

@aaronbee

Description

@aaronbee

I wasn't exactly sure which repo to put this issue.

There is some code in gnmi/tunnel.go that busy loops and is data racey.

	// Monitor if the client is registered.
	go func() {
		for {
			if client.Registered {
				started <- true
				return
			}
		}
	}()

	if err = client.Run(ctx); err != nil {
		chErr <- err
		return
	}

This will burn a CPU core waiting for client.Registered to be set and it is an unprotected read of client.Registered, which is written by a different goroutine.

One way tunnel.Client could be refactored to avoid this issue entirely is to split up Client.Run into two functions. One function does the setup and then returns (what is currently in the body Client.Run before the call to Client.start). Another that runs the proxy client, what currently exists in Client.start. With these two separate functions the above code in from the gnmi repository could be simply:

if err := client.Register(); err != nil { return err }
started <- true
return client.Run()

(I'm also not a fan of error channels. Most of the time code with error channels can be refactored into a function that just returns an error.)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions