thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (THRIFT-4552) why acquire a lock in TSimpleServer implementation for Go?
Date Tue, 17 Apr 2018 10:34:00 GMT

    [ 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:
users@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)

Mime
View raw message