fishy commented on a change in pull request #2497:
URL: https://github.com/apache/thrift/pull/2497#discussion_r781403421
##########
File path: lib/go/thrift/simple_server.go
##########
@@ -192,13 +195,27 @@ func (p *TSimpleServer) innerAccept() (int32, error) {
return 0, err
}
if client != nil {
- p.wg.Add(1)
+ ctx, cancel := context.WithCancel(context.Background())
+ p.wg.Add(2)
Review comment:
style nit: it's probably less error prone to use `wg.Add(1)` right
before every `go`, e.g. write it this way:
```go
ctx, cancel := context.WithCancel(context.Background())
p.wg.Add(1)
go func() {
defer p.wg.Done()
// actual work
}()
p.wg.Add(1)
go func() {
defer p.wg.Done()
// actual work
}()
```
this avoids the potential bug that someone adds another goroutine in the
future but forgot to change the `2` to `3`.
also style-wise, it's really better to always immediately do `defer
cancel()` right after `ctx, cancel := ...` (which is a style guide to avoid
resource leaks), which is not possible in the current 2 separated goroutines
way but possible if you wrap the second goroutine as a goroutine inside the
first one (which was what I suggested in
https://github.com/apache/thrift/pull/2497#discussion_r779887382 :) )
I don't feel too strongly for either of the nits here, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
