TBufferedServer: Avoid channel close/send race on Stop

[chlunde]

TBufferedServer.Close() may close the data channel before
Serve detects it. Move chan close() to Serve to prevent
a panic caused by sending on a closed channel.

See also golang/go#27769 (comment)

=== RUN   TestTBufferedServer_SendReceive
==================
WARNING: DATA RACE
Write at 0x00c000074850 by goroutine 9:
  runtime.closechan()
      /home/travis/.gimme/versions/go1.15.2.linux.amd64/src/runtime/chan.go:352 +0x0
  github.com/jaegertracing/jaeger/cmd/agent/app/servers.(*TBufferedServer).Stop()
...
Previous read at 0x00c000074850 by goroutine 10:
  runtime.chansend()
      /home/travis/.gimme/versions/go1.15.2.linux.amd64/src/runtime/chan.go:158 +0x0
  github.com/jaegertracing/jaeger/cmd/agent/app/servers.(*TBufferedServer).Serve()
      /home/travis/gopath/src/github.com/jaegertracing/jaeger/cmd/agent/app/servers/tbuffered_server.go:97 +0x264
...
==================
    testing.go:1042: race detected during execution of test
--- FAIL: TestTBufferedServer_SendReceive (0.01s)

Closes #2577

Signed-off-by: Carl Henrik Lunde [email protected]

[chlunde]

The downside is that the channel will not be closed if Serve is never called, but I can't see any way this will happen with the current code.

[yurishkuro]

Your PR is changing how Close and Read are interacting, not Close and Send.

[chlunde]

Not sure if I follow, I believe this is what happens on dataChan:

Goroutine 1                                                  | Goroutine 2
-------------------------------------------------------------|------------------------------
// Serve initiates the readers and starts serving traffic    |
func (s *TBufferedServer) Serve() {                          |
  atomic.StoreUint32(&s.serving, 1)                          |
  for atomic.LoadUint32(&s.serving) == 1 { // IsServing      |
    readBuf := s....                                         |
    // ... context switch ....  ---->                        | // Stop stops the serving of traffic
                                                             | // and waits until the queue is
                                                             | // emptied by the readers
                                                             | func (s *TBufferedServer) Stop() {
                                                             |   atomic.StoreUint32(&s.serving, 0)
                                                             |   _ = s.transport.Close()
                                                             |   close(s.dataChan)
                                                             | // <----- ... context switch back ...
                                                             |
    s.dataChan <- readBuf // line 97 - panic, send on closed |
  }                                                          |
}                                                            |

After patch, 4000 runs no error

go test -test.count=4000 ./cmd/agent/app/servers/
ok  	github.com/jaegertracing/jaeger/cmd/agent/app/servers	45.526s

Before patch, typically it fails before 30 runs:

go test -test.count=30 ./cmd/agent/app/servers/
panic: send on closed channel

goroutine 9 [running]:
github.com/jaegertracing/jaeger/cmd/agent/app/servers.(*TBufferedServer).Serve(0xc000170bd0)
	.../src/jaeger/cmd/agent/app/servers/tbuffered_server.go:97 +0x129
created by github.com/jaegertracing/jaeger/cmd/agent/app/servers.TestTBufferedServer_SendReceive
	.../src/jaeger/cmd/agent/app/servers/tbuffered_server_test.go:47 +0x1bb
FAIL	github.com/jaegertracing/jaeger/cmd/agent/app/servers	0.017s
FAIL

you're correct

[codecov]

Codecov Report

Merging #2583 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2583      +/-   ##
==========================================
- Coverage   95.31%   95.30%   -0.02%     
==========================================
  Files         208      208              
  Lines        9281     9281              
==========================================
- Hits         8846     8845       -1     
- Misses        356      357       +1     
  Partials       79       79              

Impacted Files	Coverage Δ
cmd/agent/app/servers/tbuffered_server.go	100.00% <100.00%> (ø)
...lugin/sampling/strategystore/adaptive/processor.go	99.07% <0.00%> (-0.93%)	⬇️
plugin/storage/integration/integration.go	77.90% <0.00%> (+0.55%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f954fe...0d43fff. Read the comment docs.

[yurishkuro]

Thanks!