[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)
> }
> }()
> }
> 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.
>
[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 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 un
[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] [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] [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)
