Почему не срабатывает блокировка с помощью sync.Mutex?

Question

[Илья Бондаренко]

Есть следующий код:

type rawCatalogArray struct {
	sync.Mutex
	TagArray []RawCatalogTag   `xml:",any"`
	TagMap   map[string]string `xml:"-"`
}

func (a *rawCatalogArray) GetKeyValue(key string) (value string, e error) {
	a.Lock()
	defer a.Unlock()
	var exist bool
	value, exist = a.TagMap[key]
	if exist {
		return
	}
	e = catErrors.New("\"%s\" is not a valid tag.", key)
	return
}

Который исполняется севером. При тестировании через `go test` и выполнение единичного запроса все работает отлично! Но при обработке параллельных запросов сервер падает:

fatal error: concurrent map read and map write

goroutine 81 [running]:
runtime.throw(0x93a23d, 0x21)
/usr/local/Cellar/go/1.7.3/libexec/src/runtime/panic.go:566 +0x95 fp=0xc4203dd138 sp=0xc4203dd118
runtime.mapaccess2_faststr(0x84e260, 0xc42021a300, 0x9291be, 0xc, 0xc42006b768, 0xa)
/usr/local/Cellar/go/1.7.3/libexec/src/runtime/hashmap_fast.go:306 +0x76e fp=0xc4203dd1d0 sp=0xc4203dd138
store/catalog.(*rawCatalogArray).GetKeyValue(0xc42006b768, 0x9291be, 0xc, 0x0, 0x0, 0x0, 0x0)
/Users/ibondarenko/Projects/Go/store/src/store/catalog/raw.go:298 +0x154 fp=0xc4203dd258 sp=0xc4203dd1d0
store/catalog.(*ClientV3Config).SetRotateTimer(0xc4203dd3b0, 0xc42006b700)
/Users/ibondarenko/Projects/Go/store/src/store/catalog/client_v3.go:113 +0x58 fp=0xc4203dd2a8 sp=0xc4203dd258
store/catalog.(*ClientV3).SetConfig(0xc4203dd2f8, 0xc42006b700)
/Users/ibondarenko/Projects/Go/store/src/store/catalog/client_v3.go:85 +0xe5 fp=0xc4203dd2d0 sp=0xc4203dd2a8
store/catalog.NewClientV3(0xc42006b700, 0xc4200ab130, 0x4b, 0xc4200ab1d0, 0x4e, 0xc42013ec98, 0x7, 0xc42013ecc8, 0x7, 0xc42013ecf8, ...)
/Users/ibondarenko/Projects/Go/store/src/store/catalog/client_v3.go:73 +0x96 fp=0xc4203dd2f0 sp=0xc4203dd2d0
store/responders.(*Client).ProcessV3(0xc4203dd958, 0xc42006b700)
/Users/ibondarenko/Projects/Go/store/src/store/responders/client.go:262 +0x54 fp=0xc4203dd800 sp=0xc4203dd2f0
main.httpHandler(0xcdea80, 0xc4201e6ea0, 0xc420206ff0)
/Users/ibondarenko/Projects/Go/store/src/store/client.go:96 +0x363 fp=0xc4203ddb98 sp=0xc4203dd800
main.(*Bootstrap).ServeHTTP(0xd21e98, 0xcdea80, 0xc4201e6ea0, 0xc420206ff0)
/Users/ibondarenko/Projects/Go/store/src/store/client.go:40 +0x3ab fp=0xc4203ddc08 sp=0xc4203ddb98
net/http.serverHandler.ServeHTTP(0xc4201e0800, 0xcdea80, 0xc4201e6ea0, 0xc420206ff0)
/usr/local/Cellar/go/1.7.3/libexec/src/net/http/server.go:2202 +0xbc fp=0xc4203ddc50 sp=0xc4203ddc08
net/http.(*conn).serve(0xc4202f0f00, 0xcdf440, 0xc4203e42c0)
/usr/local/Cellar/go/1.7.3/libexec/src/net/http/server.go:1579 +0x5f7 fp=0xc4203ddf88 sp=0xc4203ddc50
runtime.goexit()
/usr/local/Cellar/go/1.7.3/libexec/src/runtime/asm_amd64.s:2086 +0x1 fp=0xc4203ddf90 sp=0xc4203ddf88
created by net/http.(*Server).Serve
/usr/local/Cellar/go/1.7.3/libexec/src/net/http/server.go:2293 +0x541

В Go новичок, проект старый, пытаюсь разобраться. Почему код выполняется до чтения из словаря, если mutex заблокирован? Подскажите куда копать.

Answer 1

[Илья]

Потому что используйте RWMutex
У вас где-то пишется в TagMap без использования мьютекса.

Comments

[Oleg Shevelev]

Тогда стоит привести пример что бы топикпастеру было понятней:

...
import (
    "sync"
)
...
type rawCatalogArray struct {
  sync.RWMutex
  TagArray []RawCatalogTag   `xml:",any"`
  TagMap   map[string]string `xml:"-"`
}

func (a *rawCatalogArray) SetKeyValue(key, value string) {
  a.Lock()
  defer a.Unlock()
  a.TagMap[key] = value
}

func (a *rawCatalogArray) GetKeyValue(key string) (value string, e error) {
  a.RLock()
  defer a.RUnlock()
  var exist bool
  value, exist = a.TagMap[key]
  if exist {
    return
  }
  e = catErrors.New("\"%s\" is not a valid tag.", key)
  return
}
...

Лично у меня возникли вопросы:
- почему код не оформлен в отдельный тест, а вырезан из контекста
- что это за catErrors.New
- зачем такие названия функций (GetKeyValue) и переменных (обычных err error, ok bool не достаточно)
- почему изначально не был показан метод Set()

Но это если придираться:)

[murlogen]

Бредятину пишете.
RWMutex обеспечивает меньшую защиту чем обычный Mutex
То есть с обычным Mutex (если он корректно использован) как минимум не хуже блокируется чем с RWMutex

[Илья]

murlogen: Если возникает сильная нагрузка, где много читают и много изменяют, то следует использовать rw, для скорости. И у меня написано, что в мап пишется где-то без блокировки. Этой информации достаточно, чтобы начать гуглить и понять. А про обеспечение меньшей защиты - вот это бред.

[murlogen]

Илья:
Спасибо, я знаю что такое RWMutex

В данном случае - проблема не из-за этого.
В крайнем случае с Mutex должно работать медленнее.

Но уж никак не вываливаться

Еще раз для непонятливых:

Если Mutex не блокирует нормально, то RWMutex не решит проблему в корне.
Напротив - RWMutex проблему спрячет и ты ее не выявишь.

[Илья Бондаренко]

Oleg Shevelev: Извините, что код вырезал. Там просто проект такой, либо из контекста вырывать, либо код всего проекта выкладывать. `catErrors` это пакет в котором интерфейс `error` реализован и еще пару фич для сбора статистики по ошибкам. Все остальное легаси. Проект вообще очень странный с точки зрения архитектуры, но как говорится, работаем с тем что есть.
murlogen: Полностью с Вами согласен.

Наверное стоило еще привесит вот это:

type RawCatalog struct {
	XMLName     xml.Name `xml:"catalog"`
	CatalogId      string            `xml:"catalogId,omitempty"`
	Keys              rawCatalogArray   `xml:"keys,omitempty"`
	Metadata      rawCatalogArray   `xml:"metadata,omitempty"`
	Theme          rawCatalogArray   `xml:"theme,omitempty"`
	Items            []RawCatalogItem  `xml:"item"`
	Revision       string
}

и сказать что есть map[string]* RawCatalog из которого при запросе берется каталог.

Проблему решил изменив map[string]* RawCatalog на map[string]RawCatalog. Как я понимаю теперь при каждом запросе, когда я достаю каталог из этого словаря, я работаю с копией каталога. Также заменил в RawCatalog rawCatalogArray на поинтер *rawCatalogArray. Теперь мютекс работает, но понимание почему все равно кто-то конкурентно лезет читать/писать в словарь не пришло: в коде handler'a запроса нет горутин, там все последовательно.

[Илья Бондаренко]

Илья Зря Вы смеетесь) https://play.golang.org/p/jj5KJFYTpQ Выполните локально (в сандбоксе runtime.GOMAXPROCS не изменяется ). Разница только в том что используется поинтер или нет.

Answer 2

[bnytiki]

Блокировка должна работать нормально.
Поставьте перед и после a.Lock()

log.Printf("Call a.Lock(). Before")
и
log.Printf("Call a.Lock(). After")
и убедитесь в этом.

Этот код вообще не должен компилироваться.
Вы привели не полный код?

Comments

[Илья Бондаренко]

Понятно что log.Printf() будет выполняться. А чтобы код компилироваться я должен весь проект выложить:) думаю работодатель не будет доволен.

Answer 3

[Евгений Иваха]

В стэк трейсе, в принципе, пишется где предыдущее обращение к мапе выполнено. Стоит внимательно туда посмотреть. Однозначно кто-то ещё работает с мапой.