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

Reply via email to