honeybadger-io / honeybadger-go

Send Go (golang) panics and errors to Honeybadger.
https://www.honeybadger.io/
MIT License
34 stars 15 forks source link

Concurrent map writes #31

Closed evt closed 6 years ago

evt commented 6 years ago

Greetings,

Looks like there is an issue with type hash map[string]interface{} used in

type Context hash

// Update applies the values in other Context to context.
func (context Context) Update(other Context) {
    for k, v := range other {
        context[k] = v
    }
}

Maps are not safe for concurrent use as per https://golang.org/doc/faq#atomic_maps and so we often face the following error:

fatal error: concurrent map writes

goroutine 1387 [running]:
runtime.throw(0x9de572, 0x15)
    /usr/local/go/src/runtime/panic.go:605 +0x95 fp=0xc420049c58 sp=0xc420049c38 pc=0x42c115
runtime.mapassign_faststr(0x942180, 0xc42006db60, 0xc421a877c0, 0xd, 0x900480)
    /usr/local/go/src/runtime/hashmap_fast.go:861 +0x4da fp=0xc420049cd8 sp=0xc420049c58 pc=0x40e99a
github.com/honeybadger-io/honeybadger-go.Context.Update(0xc42006db60, 0xc4223a43f0)
    /go/src/github.com/honeybadger-io/honeybadger-go/context.go:9 +0xd7 fp=0xc420049d80 sp=0xc420049cd8 pc=0x7842b7
github.com/honeybadger-io/honeybadger-go.(*Client).SetContext(0xc4200894c0, 0xc4223a43f0)
    /go/src/github.com/honeybadger-io/honeybadger-go/client.go:39 +0x3c fp=0xc420049da0 sp=0xc420049d80 pc=0x7826dc
github.com/honeybadger-io/honeybadger-go.SetContext(0xc4223a43f0)
    /go/src/github.com/honeybadger-io/honeybadger-go/honeybadger.go:60 +0x37 fp=0xc420049dc0 sp=0xc420049da0 pc=0x784c37

Probably better replace with sync.Map or something like that.

joshuap commented 6 years ago

@evt thanks for the report (and sorry for not replying sooner--I just returned from some time off).

Do you have a way to reproduce the error by any chance?

pauldub commented 6 years ago

@joshuap this provides a simple enough test case:

package main

import (
    "fmt"
    "sync"

    "github.com/honeybadger-io/honeybadger-go"
)

func main() {
    var (
        wg            sync.WaitGroup
        context       = honeybadger.Context{}
        updateContext = honeybadger.Context{}
    )

    for i := 0; i < 1000000; i++ {
        updateContext[fmt.Sprintf("%d", i)] = i
    }

    wg.Add(2)

    go update(wg, context, updateContext)
    go update(wg, context, updateContext)

    wg.Wait()
}

func update(wg sync.WaitGroup, context honeybadger.Context, updateContext honeybadger.Context) {
    context.Update(updateContext)
    wg.Done()
}
odarriba commented 6 years ago

Any chances of solving this? We are facing same error when running a high concurrency app 😢

joshuap commented 6 years ago

@odarriba I'll take a look at it today.

joshuap commented 6 years ago

@odarriba can you take a look at https://github.com/honeybadger-io/honeybadger-go/commit/1340b32ca4b7551867a7df97ee561e736689ddc0 (read the commit message for extra info) and tell me if it will solve this issue for you?