francoispqt / gojay

high performance JSON encoder/decoder with stream API for Golang
MIT License
2.11k stars 112 forks source link

panic: close of closed channel #97

Closed OGKevin closed 5 years ago

OGKevin commented 5 years ago

When i run

    enc := gojay.Stream.NewEncoder(buf).NConsumer(10).CommaDelimited()
    go enc.EncodeStream(processListCh)
    <- enc.Done()

in a loop of 1 sec interval, i manage to get

panic: close of closed channel

goroutine 591 [running]:
.../vendor/github.com/francoispqt/gojay.(*StreamEncoder).Cancel(0xc00014c570, 0x0, 0x0)
        .../vendor/github.com/francoispqt/gojay/encode_stream.go:133 +0xa3
...../vendor/github.com/francoispqt/gojay.consume(0xc00014c570, 0xc00014c720, 0x140f2a0, 0xc000122300)
        ..../vendor/github.com/francoispqt/gojay/encode_stream.go:199 +0xeb
created by .../vendor/github.com/francoispqt/gojay.(*StreamEncoder).EncodeStream
      ..../vendor/github.com/francoispqt/gojay/encode_stream.go:53 +0x1b3
exit status 2

This is caused by https://github.com/francoispqt/gojay/blob/90d953358b68936b4791b8db205b3a5918b1b7a5/encode_stream.go#L124-L136

on line 133, however i've not managed to locate how it got closed right after the <-done check and before the close() call. However, i can constantly reproduce this 🤔 I have a feeling there should be a mutex with write lock to prevent this from happening.

OGKevin commented 5 years ago

Found it, replace close(ch) with ch <- nil for the chan in question and it fixes it.

        //defer close(ch)
        defer func() {
            ch <- nil
        }()

closing the channel will make a select statement constantly select the closed channel e.g:

func (p ProcessListChan) MarshalStream(enc *gojay.StreamEncoder) {
    select {
    case <-enc.Done():
        return
    case o := <-p:
        enc.AddObject(o)
    }
}

case <-p is activated rapidly causing the panic.

I would say that gojay needs to implement better mutex lock to ensure only 1 go routine can close the done chan as users expect contex.Context to be able to work good under concurrent load.