[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16440393#comment-16440393 ]
ASF GitHub Bot commented on THRIFT-4552: ---------------------------------------- dcelasun commented on issue #1542: THRIFT-4552: remove unneccessary lock URL: https://github.com/apache/thrift/pull/1542#issuecomment-381844049 Unfortunately, this commit [reintroduced](https://travis-ci.org/apache/thrift/jobs/366167870) the race condition: ```sh GOPATH=`pwd` /usr/local/bin/go test -race ./thrift ================== WARNING: DATA RACE Write at 0x00c420248810 by goroutine 46: internal/race.Write() /usr/local/go/src/internal/race/race.go:41 +0x38 sync.(*WaitGroup).Wait() /usr/local/go/src/sync/waitgroup.go:127 +0xf3 _/thrift/src/lib/go/thrift.(*TSimpleServer).Stop() /thrift/src/lib/go/thrift/simple_server.go:174 +0x91 _/thrift/src/lib/go/thrift.TestWaitRace() /thrift/src/lib/go/thrift/simple_server_test.go:127 +0x1a3 testing.tRunner() /usr/local/go/src/testing/testing.go:777 +0x16d Previous read at 0x00c420248810 by goroutine 78: internal/race.Read() /usr/local/go/src/internal/race/race.go:37 +0x38 sync.(*WaitGroup).Add() /usr/local/go/src/sync/waitgroup.go:70 +0x16e _/thrift/src/lib/go/thrift.(*TSimpleServer).innerAccept() /thrift/src/lib/go/thrift/simple_server.go:137 +0xe7 _/thrift/src/lib/go/thrift.(*TSimpleServer).AcceptLoop() /thrift/src/lib/go/thrift/simple_server.go:150 +0x3c _/thrift/src/lib/go/thrift.(*TSimpleServer).Serve() /thrift/src/lib/go/thrift/simple_server.go:165 +0x86 Goroutine 46 (running) created at: testing.(*T).Run() /usr/local/go/src/testing/testing.go:824 +0x564 testing.runTests.func1() /usr/local/go/src/testing/testing.go:1063 +0xa4 testing.tRunner() /usr/local/go/src/testing/testing.go:777 +0x16d testing.runTests() /usr/local/go/src/testing/testing.go:1061 +0x4e1 testing.(*M).Run() /usr/local/go/src/testing/testing.go:978 +0x2cd main.main() _testmain.go:274 +0x22a Goroutine 78 (running) created at: _/thrift/src/lib/go/thrift.TestWaitRace() /thrift/src/lib/go/thrift/simple_server_test.go:125 +0x190 testing.tRunner() /usr/local/go/src/testing/testing.go:777 +0x16d ================== --- FAIL: TestWaitRace (0.00s) testing.go:730: race detected during execution of test FAIL FAIL _/thrift/src/lib/go/thrift 0.114s ``` ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org > why acquire a lock in TSimpleServer implementation for Go? > ---------------------------------------------------------- > > Key: THRIFT-4552 > URL: https://issues.apache.org/jira/browse/THRIFT-4552 > Project: Thrift > Issue Type: Improvement > Affects Versions: 0.11.0 > Reporter: gansteed > Priority: Minor > > I've sent a email to groups, but I think maybe here will be better? > > I'm using Thrift and I'm reading thrift implementation for Go, I found code > in `TSimpleServer.AcceptLoop` like this: > > {code:go} > func (p *TSimpleServer) AcceptLoop() error { > for { > client, err := p.serverTransport.Accept() > p.mu.Lock() > if atomic.LoadInt32(&p.closed) != 0 { > return nil > } > if err != nil { > return err > } > if client != nil { > p.wg.Add(1) > go func() { > defer p.wg.Done() > if err := p.processRequests(client); err != > nil { > log.Println("error processing > request:", err) > } > }() > } > p.mu.Unlock() > } > } > {code} > > every time it accept a request,it: > > 1. read if protocol had been closed, this step is atomic, it does not need a > lock. > 2. p.wg.Add(1) to accumulate a goroutine? this step is atomic, too, it does > not need a lock > 3. after processor processed the request, it do p.wg.Done(), it's atomic, > too, and it does not need a lock. > > by the way, it seems that `p.wg.Done()` do not need to put in defer? just put > it after p.processRequests(client)? > > so is there any particular to do it in this way?if not, I would like to > submit a PR to reduce unneccessary performance overhead in TSimpleServer > implementation. > > -- This message was sent by Atlassian JIRA (v7.6.3#76005)