You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Can Celasun (JIRA)" <ji...@apache.org> on 2018/04/13 05:49:00 UTC
[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)
> }
> }()
> }
> 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)