[jira] [Closed] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Can Celasun closed THRIFT-4552. --- Resolution: Invalid See https://issues.apache.org/jira/browse/THRIFT-4557 > 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)
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16440728#comment-16440728 ] ASF GitHub Bot commented on THRIFT-4552: jiajunhuang closed pull request #1542: THRIFT-4552: remove unneccessary lock URL: https://github.com/apache/thrift/pull/1542 This is a PR merged from a forked repository. As GitHub hides the original diff on merge, it is displayed below for the sake of provenance: As this is a foreign pull request (from a fork), the diff is supplied below (as it won't show otherwise due to GitHub magic): diff --git a/lib/go/thrift/simple_server.go b/lib/go/thrift/simple_server.go index 6035802516..4b42387ee3 100644 --- a/lib/go/thrift/simple_server.go +++ b/lib/go/thrift/simple_server.go @@ -34,7 +34,6 @@ import ( type TSimpleServer struct { closed int32 wg sync.WaitGroup - mu sync.Mutex processorFactory TProcessorFactory serverTransportTServerTransport @@ -127,8 +126,6 @@ func (p *TSimpleServer) Listen() error { func (p *TSimpleServer) innerAccept() (int32, error) { client, err := p.serverTransport.Accept() - p.mu.Lock() - defer p.mu.Unlock() closed := atomic.LoadInt32(&p.closed) if closed != 0 { return closed, nil @@ -139,10 +136,10 @@ func (p *TSimpleServer) innerAccept() (int32, error) { 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.wg.Done() }() } return 0, nil @@ -170,12 +167,9 @@ func (p *TSimpleServer) Serve() error { } func (p *TSimpleServer) Stop() error { - p.mu.Lock() - defer p.mu.Unlock() - if atomic.LoadInt32(&p.closed) != 0 { + if swapped := atomic.CompareAndSwapInt32(&p.closed, 0, 1); !swapped { return nil } - atomic.StoreInt32(&p.closed, 1) p.serverTransport.Interrupt() p.wg.Wait() return nil diff --git a/lib/go/thrift/simple_server_test.go b/lib/go/thrift/simple_server_test.go index 58149a8e66..4dfcfd79fa 100644 --- a/lib/go/thrift/simple_server_test.go +++ b/lib/go/thrift/simple_server_test.go @@ -20,9 +20,9 @@ package thrift import ( - "testing" "errors" "runtime" + "testing" ) type mockServerTransport struct { diff --git a/tutorial/go/src/server.go b/tutorial/go/src/server.go index e4c4b97071..cc29450480 100644 --- a/tutorial/go/src/server.go +++ b/tutorial/go/src/server.go @@ -40,7 +40,7 @@ func runServer(transportFactory thrift.TTransportFactory, protocolFactory thrift } else { transport, err = thrift.NewTServerSocket(addr) } - + if err != nil { return err } 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) > } >
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16440727#comment-16440727 ] ASF GitHub Bot commented on THRIFT-4552: jiajunhuang commented on issue #1542: THRIFT-4552: remove unneccessary lock URL: https://github.com/apache/thrift/pull/1542#issuecomment-381940834 hello, I've made a mistake: the race condition occurred by: `p.wg.Wait` is called before `p.wg.Add`. So the lock should exist to make the `Stop` and `innerAccept` atomic. 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)
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ 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 t
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16439025#comment-16439025 ] gansteed commented on THRIFT-4552: -- hello, the PR failed because of C++ tests: https://travis-ci.org/apache/thrift/builds/366167861?utm_source=github_status&utm_medium=notification I've run test cases of Go, it's passed. Are there any solutions? or just ignore the C++ errors? > 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)
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16437451#comment-16437451 ] ASF GitHub Bot commented on THRIFT-4552: jiajunhuang opened a new pull request #1542: THRIFT-4552: remove unneccessary lock URL: https://github.com/apache/thrift/pull/1542 1. use CAS operation instead of lock 2. remove defer to reduce unneccessary performance consume 3. go fmt those code 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)
[jira] [Updated] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] James E. King, III updated THRIFT-4552: --- Priority: Minor (was: Major) > 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)
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16437127#comment-16437127 ] Can Celasun commented on THRIFT-4552: - OK, go ahead and send a PR. Make sure your commit message begins with THRIFT-4552. > 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: Major > > 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)
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16437123#comment-16437123 ] gansteed commented on THRIFT-4552: -- That's what I mean in last comment, just like you demo. in https://issues.apache.org/jira/browse/THRIFT-4243 , it does things below: - go serv.Serve() - serv.Stop() wg.Add and wg.Done will only occurred in `innerAccept` which is called by `AcceptLoop` wg.Wait will only occurred in `Stop` so, if you call `go serv.Serve()`, it will spawn a goroutine, and the goroutine will block on `client, err := p.serverTransport.Accept()` until there comes a new request. and the main goroutine which execute `serv.Stop` will do: 1. load p.closed and check if it is already closed, if not, set it as closed 2. call `p.serverTransport.Interrupt()` 3. call `p.wg.Wait` to wait all spawned goroutines(which handle RPC requests) the only possible race panic will occur in such a way: 1. main goroutine call `serv.Stop`, which then call `if atomic.LoadInt32(&p.closed) != 0`, in the mean time, there comes a request, and the goroutine which handle it call `closed := atomic.LoadInt32(&p.closed)` 2. the main goroutine call `atomic.StoreInt32(&p.closed, 1)`, `p.serverTransport.Interrupt()`, then `p.wg.Wait()` 3. and the goroutine which handle the request call `p.wg.Add(1)` then it panic! so, the solution is use https://golang.org/pkg/sync/atomic/#CompareAndSwapInt32 instead of two steps(first load, then store). may I sent a PR in Github to solve this problem? > 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: Major > > 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)
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16437025#comment-16437025 ] Can Celasun commented on THRIFT-4552: - Let me try to give an example: {code:go} func main() { wg := &sync.WaitGroup{} go work(wg) wg.Wait() } func work(wg *sync.WaitGroup) { wg.Add(1) // stuff defer wg.Done() } {code} Here, there is a race conditon because if {{wg.Wait()}} executes before {{wg.Add(1)}}, Wait will panic as [documented|https://godoc.org/sync#WaitGroup.Add]: {quote}Note that calls with a positive delta that occur when the counter is zero must happen before a Wait.{quote} Still, I agree with you that this has a non-neglible performance impact. Let me try a few things and post back there. > 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: Major > > 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)
[jira] [Updated] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Can Celasun updated THRIFT-4552: Description: 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. was: 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: ```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() } } ``` 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. > 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: Major > > 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) > } >
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16436867#comment-16436867 ] gansteed commented on THRIFT-4552: -- hello, thanks for your reply, I don't fully understand the issue. does it means that, if one stop the server by doing `serv.Stop()` then a request is come, and the server will panic? that's wired, every time server accept a request, it will check if the server is closed, right? > 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: Major > > 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: > > ```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() > } > } > ``` > > 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)
[jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
[ https://issues.apache.org/jira/browse/THRIFT-4552?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16435554#comment-16435554 ] Can Celasun commented on THRIFT-4552: - See [THRIFT-4243|https://issues.apache.org/jira/browse/THRIFT-4243] for why the lock & atomic value exist. Maybe a better approach is to do {{p.wg.Add(1)}} immediately after creating the wait group (and a matching {{p.wg.Done()}}), so we won't run into the race mentioned in THRIFT-4243. > 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: Major > > 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: > > ```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() > } > } > ``` > > 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)
[jira] [Created] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
gansteed created THRIFT-4552: Summary: 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 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: ```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() } } ``` 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)
why acquire a lock in TSimpleServer implementation for Go?
hi, all: I'm using Thrift and I'm reading thrift implementation for Go, I found code in `TSimpleServer.AcceptLoop` like this: ```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() } } ``` 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.