[
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
serverTransport TServerTransport
@@ -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.
> 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)
