francoispqt / gojay

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

concurrent unsafety of gojay.Marshal is not clearly documented #83

Closed neex closed 5 years ago

neex commented 5 years ago

Hello,

I think that the fact that gojay.Marshal is not safe for concurrent usage from multiple goroutines should be explicitly mentioned both in the readme and godoc.

Now it is stated that Marshal borrows an encoder and releases it back no matter what. While an attentive reader will have realised that it introduces a race (because same encoder could be reused by another goroutine and the buffer will get corrupted), the race is easily overseen problem as everything seems to be working most of the time, so it should be mentioned (may be even in bold and with some exclamation marks).

Example program that shows the race exists:

package main

import (
    "log"
    "strconv"
    "time"

    "github.com/francoispqt/gojay"
)

func proeb(num int) {
    for {
        b, err := gojay.Marshal(num)
        if err != nil {
            log.Fatal(err)
        }

        s := string(b)
        if n, err := strconv.Atoi(s); err != nil || n != num {
            log.Printf("caught race: %v %v", s, num)
        }
    }
}

func main() {
    for i := 0; i < 100; i++ {
        go proeb(i)
    }
    time.Sleep(100 * time.Second)
}
francoispqt commented 5 years ago

This is correct, I have just pushed a fix to master which effectively creates a new buffer when borrowing. Previously it was doing enc.buf = enc.buf[:0] which is wrong. I am patching versions with the fix.

If the fix works for you, please close the issue.

Note: the problem doesn't happen if you use an io.Writer with the Encode interface because it writes to the writer before releasing.

neex commented 5 years ago

I thought it was the intended behaviour, just not documented.

Thanks for the fix!