server: move procCh init into Server.serveImpl #106943
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
ecwall
changed the title
There was a problem hiding this comment.
nice, seems like a great simplification. left one suggestion
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ecwall)
pkg/sql/pgwire/server.go line 972 at r1 (raw file):
// procCh is the channel on which we'll receive the termination signal from // the command processor. procCh := make(chan struct{})
nit: this could be a bounded channel of size 1
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/server.go line 972 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: this could be a bounded channel of size 1
The channel can be unbuffered because nothing is ever sent or received from it. It just blocks until it is closed like this example https://medium.com/@matryer/golang-advent-calendar-day-two-starting-and-stopping-things-with-a-signal-channel-f5048161018.
Is there some other reason to give it size 1?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/server.go line 972 at r1 (raw file):
Previously, ecwall (Evan Wall) wrote…
The channel can be unbuffered because nothing is ever sent or received from it. It just blocks until it is closed like this example https://medium.com/@matryer/golang-advent-calendar-day-two-starting-and-stopping-things-with-a-signal-channel-f5048161018.
Is there some other reason to give it size 1?
my thinking was just to prevent an accidental memory leak, in case someone does start sending to it.
another option is to use a WaitGroup. but i don't feel strongly about it - they just are less confusing to me than channels.
Informs #105448 This changes `procCh` to a `sync.WaitGroup` because the channel is never read from and moves initialization into `Server.serveImpl`. Also `processCommandsAsync` is changed to `processCommands` and the goroutine is created inside `Server.serverImpl` to avoid needing a `procCh` parameter. Release note: None
ecwall
changed the title
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/server.go line 972 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
my thinking was just to prevent an accidental memory leak, in case someone does start sending to it.
another option is to use a WaitGroup. but i don't feel strongly about it - they just are less confusing to me than channels.
Changed to sync.WaitGroup.
|
bors r=rafiss |
|
Build succeeded: |
Informs #105448
This changes
procChto async.WaitGroupbecause the channel is never readfrom and moves initialization into
Server.serveImpl.Also
processCommandsAsyncis changed toprocessCommandsand the goroutineis created inside
Server.serverImplto avoid needing aprocChparameter.Release note: None