[
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)
