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