From f8c10fd5cfb58881d63303dff5149b0a1905c989 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 28 Jun 2022 01:23:58 +0100 Subject: [PATCH 1/4] Fix races in i18n code The recent changes to add live-reloading to the i18n translation files added a few races. This PR fixes these, adds some more comments to the code and slightly restructures a few functions. Signed-off-by: Andrew Thornton --- modules/translation/i18n/i18n.go | 242 ++++++++++++++++++++++--------- 1 file changed, 170 insertions(+), 72 deletions(-) diff --git a/modules/translation/i18n/i18n.go b/modules/translation/i18n/i18n.go index acce5f19fb0dc..e6cf78897e50a 100644 --- a/modules/translation/i18n/i18n.go +++ b/modules/translation/i18n/i18n.go @@ -25,9 +25,13 @@ var ( ) type locale struct { + // This mutex will be set if we have live-reload enabled (e.g. dev mode) + reloadMu *sync.RWMutex + store *LocaleStore langName string - textMap map[int]string // the map key (idx) is generated by store's textIdxMap + + idxToMsgMap map[int]string // the map idx is generated by store's keyToKeyIdxMap sourceFileName string sourceFileInfo os.FileInfo @@ -35,47 +39,61 @@ type locale struct { } type LocaleStore struct { - reloadMu *sync.Mutex // for non-prod(dev), use a mutex for live-reload. for prod, no mutex, no live-reload. + // This mutex will be set if we have live-reload enabled (e.g. dev mode) + reloadMu *sync.RWMutex langNames []string langDescs []string - localeMap map[string]*locale - textIdxMap map[string]int + // these need to be locked when live-reloading + localeMap map[string]*locale + trKeyToIdxMap map[string]int defaultLang string } func NewLocaleStore(isProd bool) *LocaleStore { - ls := &LocaleStore{localeMap: make(map[string]*locale), textIdxMap: make(map[string]int)} + store := &LocaleStore{localeMap: make(map[string]*locale), trKeyToIdxMap: make(map[string]int)} if !isProd { - ls.reloadMu = &sync.Mutex{} + store.reloadMu = &sync.RWMutex{} } - return ls + return store } // AddLocaleByIni adds locale by ini into the store -// if source is a string, then the file is loaded. in dev mode, the file can be live-reloaded +// if source is a string, then the file is loaded. In dev mode, this file will be checked for live-reloading // if source is a []byte, then the content is used -func (ls *LocaleStore) AddLocaleByIni(langName, langDesc string, source interface{}) error { - if _, ok := ls.localeMap[langName]; ok { +func (store *LocaleStore) AddLocaleByIni(langName, langDesc string, source interface{}) error { + if store.reloadMu != nil { + store.reloadMu.Lock() + defer store.reloadMu.Unlock() + } + if _, ok := store.localeMap[langName]; ok { return ErrLocaleAlreadyExist } - lc := &locale{store: ls, langName: langName} + l := &locale{store: store, langName: langName} + if store.reloadMu != nil { + l.reloadMu = &sync.RWMutex{} + l.reloadMu.Lock() + defer l.reloadMu.Unlock() + } + if fileName, ok := source.(string); ok { - lc.sourceFileName = fileName - lc.sourceFileInfo, _ = os.Stat(fileName) // live-reload only works for regular files. the error can be ignored + l.sourceFileName = fileName + l.sourceFileInfo, _ = os.Stat(fileName) // live-reload only works for regular files. the error can be ignored } - ls.langNames = append(ls.langNames, langName) - ls.langDescs = append(ls.langDescs, langDesc) - ls.localeMap[lc.langName] = lc + store.langNames = append(store.langNames, langName) + store.langDescs = append(store.langDescs, langDesc) + store.localeMap[l.langName] = l - return ls.reloadLocaleByIni(langName, source) + return store.reloadLocaleByIni(langName, source) } -func (ls *LocaleStore) reloadLocaleByIni(langName string, source interface{}) error { +// store.reloadMu and l.reloadMu must be locked if it exists before calling this function +// this function arguably belongs to locale rather than store but both need to be locked +func (store *LocaleStore) reloadLocaleByIni(langName string, source interface{}) error { iniFile, err := ini.LoadSources(ini.LoadOptions{ IgnoreInlineComment: true, UnescapeValueCommentSymbols: true, @@ -85,47 +103,64 @@ func (ls *LocaleStore) reloadLocaleByIni(langName string, source interface{}) er } iniFile.BlockMode = false - lc := ls.localeMap[langName] - lc.textMap = make(map[int]string) + l := store.localeMap[langName] + l.idxToMsgMap = make(map[int]string) + for _, section := range iniFile.Sections() { for _, key := range section.Keys() { + var trKey string if section.Name() == "" || section.Name() == "DEFAULT" { trKey = key.Name() } else { trKey = section.Name() + "." + key.Name() } - textIdx, ok := ls.textIdxMap[trKey] + + // Instead of storing the key strings in multiple different maps we compute a idx which will act as numeric code for key + // This reduces the size of the locale idxToMsgMaps + idx, ok := store.trKeyToIdxMap[trKey] if !ok { - textIdx = len(ls.textIdxMap) - ls.textIdxMap[trKey] = textIdx + idx = len(store.trKeyToIdxMap) + store.trKeyToIdxMap[trKey] = idx } - lc.textMap[textIdx] = key.Value() + l.idxToMsgMap[idx] = key.Value() } } iniFile = nil return nil } -func (ls *LocaleStore) HasLang(langName string) bool { - _, ok := ls.localeMap[langName] +func (store *LocaleStore) HasLang(langName string) bool { + if store.reloadMu != nil { + store.reloadMu.RLock() + defer store.reloadMu.RUnlock() + } + + _, ok := store.localeMap[langName] return ok } -func (ls *LocaleStore) ListLangNameDesc() (names, desc []string) { - return ls.langNames, ls.langDescs +func (store *LocaleStore) ListLangNameDesc() (names, desc []string) { + return store.langNames, store.langDescs } // SetDefaultLang sets default language as a fallback -func (ls *LocaleStore) SetDefaultLang(lang string) { - ls.defaultLang = lang +func (store *LocaleStore) SetDefaultLang(lang string) { + store.defaultLang = lang } // Tr translates content to target language. fall back to default language. -func (ls *LocaleStore) Tr(lang, trKey string, trArgs ...interface{}) string { - l, ok := ls.localeMap[lang] +func (store *LocaleStore) Tr(lang, trKey string, trArgs ...interface{}) string { + if store.reloadMu != nil { + store.reloadMu.RLock() + } + + l, ok := store.localeMap[lang] if !ok { - l, ok = ls.localeMap[ls.defaultLang] + l, ok = store.localeMap[store.defaultLang] + } + if store.reloadMu != nil { + store.reloadMu.RUnlock() } if ok { return l.Tr(trKey, trArgs...) @@ -133,66 +168,129 @@ func (ls *LocaleStore) Tr(lang, trKey string, trArgs ...interface{}) string { return trKey } +// this function will assume that the l.reloadMu has been RLocked if it already exists +func (l *locale) reloadIfNeeded() { + if l.store.reloadMu == nil { + return + } + + now := time.Now() + if now.Sub(l.lastReloadCheckTime) < time.Second || l.sourceFileInfo == nil || l.sourceFileName == "" { + return + } + + l.reloadMu.RUnlock() + l.reloadMu.Lock() // (NOTE: a pre-emption can occur between these two locks so we need to recheck) + + if now.Sub(l.lastReloadCheckTime) < time.Second || l.sourceFileInfo == nil || l.sourceFileName == "" { + l.reloadMu.Unlock() + l.reloadMu.RLock() + return + } + + l.lastReloadCheckTime = now + sourceFileInfo, err := os.Stat(l.sourceFileName) + if err != nil || sourceFileInfo.ModTime().Equal(l.sourceFileInfo.ModTime()) { + l.reloadMu.Unlock() + l.reloadMu.RLock() + return + } + + l.store.reloadMu.Lock() + err = l.store.reloadLocaleByIni(l.langName, l.sourceFileName) + l.store.reloadMu.Unlock() + + if err == nil { + l.sourceFileInfo = sourceFileInfo + } else { + log.Error("unable to live-reload the locale file %q, err: %v", l.sourceFileName, err) + } + + l.reloadMu.Unlock() + l.reloadMu.RLock() +} + // Tr translates content to locale language. fall back to default language. func (l *locale) Tr(trKey string, trArgs ...interface{}) string { - if l.store.reloadMu != nil { - l.store.reloadMu.Lock() - defer l.store.reloadMu.Unlock() - now := time.Now() - if now.Sub(l.lastReloadCheckTime) >= time.Second && l.sourceFileInfo != nil && l.sourceFileName != "" { - l.lastReloadCheckTime = now - if sourceFileInfo, err := os.Stat(l.sourceFileName); err == nil && !sourceFileInfo.ModTime().Equal(l.sourceFileInfo.ModTime()) { - if err = l.store.reloadLocaleByIni(l.langName, l.sourceFileName); err == nil { - l.sourceFileInfo = sourceFileInfo - } else { - log.Error("unable to live-reload the locale file %q, err: %v", l.sourceFileName, err) - } - } - } + if l.reloadMu != nil { + l.reloadMu.RLock() + defer l.reloadMu.RUnlock() + l.reloadIfNeeded() } + msg, _ := l.tryTr(trKey, trArgs...) return msg } func (l *locale) tryTr(trKey string, trArgs ...interface{}) (msg string, found bool) { trMsg := trKey - textIdx, ok := l.store.textIdxMap[trKey] + + if l.store.reloadMu != nil { + l.store.reloadMu.RLock() + } + idx, ok := l.store.trKeyToIdxMap[trKey] + if l.store.reloadMu != nil { + l.store.reloadMu.RUnlock() + } + if ok { - if msg, found = l.textMap[textIdx]; found { + if msg, found = l.idxToMsgMap[idx]; found { trMsg = msg // use current translation } else if l.langName != l.store.defaultLang { - if def, ok := l.store.localeMap[l.store.defaultLang]; ok { - return def.tryTr(trKey, trArgs...) + + // attempt to get the default language from the locale store + if l.store.reloadMu != nil { + l.store.reloadMu.RLock() + } + def, ok := l.store.localeMap[l.store.defaultLang] + if l.store.reloadMu != nil { + l.store.reloadMu.RUnlock() + } + + if ok { + if def.reloadMu != nil { + def.reloadMu.RLock() + } + if msg, found = def.idxToMsgMap[idx]; found { + trMsg = msg // use current translation + } + if def.reloadMu != nil { + def.reloadMu.RUnlock() + } } - } else if !setting.IsProd { - log.Error("missing i18n translation key: %q", trKey) } } - if len(trArgs) > 0 { - fmtArgs := make([]interface{}, 0, len(trArgs)) - for _, arg := range trArgs { - val := reflect.ValueOf(arg) - if val.Kind() == reflect.Slice { - // before, it can accept Tr(lang, key, a, [b, c], d, [e, f]) as Sprintf(msg, a, b, c, d, e, f), it's an unstable behavior - // now, we restrict the strange behavior and only support: - // 1. Tr(lang, key, [slice-items]) as Sprintf(msg, items...) - // 2. Tr(lang, key, args...) as Sprintf(msg, args...) - if len(trArgs) == 1 { - for i := 0; i < val.Len(); i++ { - fmtArgs = append(fmtArgs, val.Index(i).Interface()) - } - } else { - log.Error("the args for i18n shouldn't contain uncertain slices, key=%q, args=%v", trKey, trArgs) - break + if !found && !setting.IsProd { + log.Error("missing i18n translation key: %q", trKey) + } + + if len(trArgs) == 0 { + return trMsg, found + } + + fmtArgs := make([]interface{}, 0, len(trArgs)) + for _, arg := range trArgs { + val := reflect.ValueOf(arg) + if val.Kind() == reflect.Slice { + // before, it can accept Tr(lang, key, a, [b, c], d, [e, f]) as Sprintf(msg, a, b, c, d, e, f), it's an unstable behavior + // now, we restrict the strange behavior and only support: + // 1. Tr(lang, key, [slice-items]) as Sprintf(msg, items...) + // 2. Tr(lang, key, args...) as Sprintf(msg, args...) + if len(trArgs) == 1 { + for i := 0; i < val.Len(); i++ { + fmtArgs = append(fmtArgs, val.Index(i).Interface()) } } else { - fmtArgs = append(fmtArgs, arg) + log.Error("the args for i18n shouldn't contain uncertain slices, key=%q, args=%v", trKey, trArgs) + break } + } else { + fmtArgs = append(fmtArgs, arg) } - return fmt.Sprintf(trMsg, fmtArgs...), found } - return trMsg, found + + return fmt.Sprintf(trMsg, fmtArgs...), found } // ResetDefaultLocales resets the current default locales From 23f2a3bc886e473e7735c4ecaf9a955a1e38443a Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 28 Jun 2022 01:25:07 +0100 Subject: [PATCH 2/4] Update modules/translation/i18n/i18n.go --- modules/translation/i18n/i18n.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/translation/i18n/i18n.go b/modules/translation/i18n/i18n.go index e6cf78897e50a..7c12510212efb 100644 --- a/modules/translation/i18n/i18n.go +++ b/modules/translation/i18n/i18n.go @@ -31,7 +31,7 @@ type locale struct { store *LocaleStore langName string - idxToMsgMap map[int]string // the map idx is generated by store's keyToKeyIdxMap + idxToMsgMap map[int]string // the map idx is generated by store's trKeyToIdxMap sourceFileName string sourceFileInfo os.FileInfo From 879f49e41be89d6a99ed50f0207f7fa4ec92520b Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 28 Jun 2022 11:46:37 +0100 Subject: [PATCH 3/4] further improvements Signed-off-by: Andrew Thornton --- modules/translation/i18n/i18n.go | 103 ++++++++++++++++--------------- 1 file changed, 52 insertions(+), 51 deletions(-) diff --git a/modules/translation/i18n/i18n.go b/modules/translation/i18n/i18n.go index 7c12510212efb..47a2dc991d640 100644 --- a/modules/translation/i18n/i18n.go +++ b/modules/translation/i18n/i18n.go @@ -44,9 +44,9 @@ type LocaleStore struct { langNames []string langDescs []string + localeMap map[string]*locale - // these need to be locked when live-reloading - localeMap map[string]*locale + // this needs to be locked when live-reloading trKeyToIdxMap map[string]int defaultLang string @@ -63,11 +63,8 @@ func NewLocaleStore(isProd bool) *LocaleStore { // AddLocaleByIni adds locale by ini into the store // if source is a string, then the file is loaded. In dev mode, this file will be checked for live-reloading // if source is a []byte, then the content is used +// Note: this is not concurrent safe func (store *LocaleStore) AddLocaleByIni(langName, langDesc string, source interface{}) error { - if store.reloadMu != nil { - store.reloadMu.Lock() - defer store.reloadMu.Unlock() - } if _, ok := store.localeMap[langName]; ok { return ErrLocaleAlreadyExist } @@ -75,7 +72,7 @@ func (store *LocaleStore) AddLocaleByIni(langName, langDesc string, source inter l := &locale{store: store, langName: langName} if store.reloadMu != nil { l.reloadMu = &sync.RWMutex{} - l.reloadMu.Lock() + l.reloadMu.Lock() // Arguably this is not necessary as AddLocaleByIni isn't concurrent safe - but for consistency we do this defer l.reloadMu.Unlock() } @@ -84,27 +81,37 @@ func (store *LocaleStore) AddLocaleByIni(langName, langDesc string, source inter l.sourceFileInfo, _ = os.Stat(fileName) // live-reload only works for regular files. the error can be ignored } + var err error + l.idxToMsgMap, err = store.readIniToIdxToMsgMap(source) + if err != nil { + return err + } + store.langNames = append(store.langNames, langName) store.langDescs = append(store.langDescs, langDesc) + store.localeMap[l.langName] = l - return store.reloadLocaleByIni(langName, source) + return nil } -// store.reloadMu and l.reloadMu must be locked if it exists before calling this function -// this function arguably belongs to locale rather than store but both need to be locked -func (store *LocaleStore) reloadLocaleByIni(langName string, source interface{}) error { +// readIniToIdxToMsgMap will read a provided ini and creates an idxToMsgMap +func (store *LocaleStore) readIniToIdxToMsgMap(source interface{}) (map[int]string, error) { iniFile, err := ini.LoadSources(ini.LoadOptions{ IgnoreInlineComment: true, UnescapeValueCommentSymbols: true, }, source) if err != nil { - return fmt.Errorf("unable to load ini: %w", err) + return nil, fmt.Errorf("unable to load ini: %w", err) } iniFile.BlockMode = false - l := store.localeMap[langName] - l.idxToMsgMap = make(map[int]string) + idxToMsgMap := make(map[int]string) + + if store.reloadMu != nil { + store.reloadMu.Lock() + defer store.reloadMu.Unlock() + } for _, section := range iniFile.Sections() { for _, key := range section.Keys() { @@ -123,23 +130,29 @@ func (store *LocaleStore) reloadLocaleByIni(langName string, source interface{}) idx = len(store.trKeyToIdxMap) store.trKeyToIdxMap[trKey] = idx } - l.idxToMsgMap[idx] = key.Value() + idxToMsgMap[idx] = key.Value() } } iniFile = nil - return nil + return idxToMsgMap, nil } -func (store *LocaleStore) HasLang(langName string) bool { +func (store *LocaleStore) idxForTrKey(trKey string) (int, bool) { if store.reloadMu != nil { store.reloadMu.RLock() defer store.reloadMu.RUnlock() } + idx, ok := store.trKeyToIdxMap[trKey] + return idx, ok +} +// HasLang reports if a language is available in the store +func (store *LocaleStore) HasLang(langName string) bool { _, ok := store.localeMap[langName] return ok } +// ListLangNameDesc reports if a language available in the store func (store *LocaleStore) ListLangNameDesc() (names, desc []string) { return store.langNames, store.langDescs } @@ -151,26 +164,21 @@ func (store *LocaleStore) SetDefaultLang(lang string) { // Tr translates content to target language. fall back to default language. func (store *LocaleStore) Tr(lang, trKey string, trArgs ...interface{}) string { - if store.reloadMu != nil { - store.reloadMu.RLock() - } - l, ok := store.localeMap[lang] if !ok { l, ok = store.localeMap[store.defaultLang] } - if store.reloadMu != nil { - store.reloadMu.RUnlock() - } + if ok { return l.Tr(trKey, trArgs...) } return trKey } +// reloadIfNeeded will check if the locale needs to be reloaded // this function will assume that the l.reloadMu has been RLocked if it already exists func (l *locale) reloadIfNeeded() { - if l.store.reloadMu == nil { + if l.reloadMu == nil { return } @@ -196,16 +204,16 @@ func (l *locale) reloadIfNeeded() { return } - l.store.reloadMu.Lock() - err = l.store.reloadLocaleByIni(l.langName, l.sourceFileName) - l.store.reloadMu.Unlock() - + idxToMsgMap, err := l.store.readIniToIdxToMsgMap(l.sourceFileName) if err == nil { - l.sourceFileInfo = sourceFileInfo + l.idxToMsgMap = idxToMsgMap } else { - log.Error("unable to live-reload the locale file %q, err: %v", l.sourceFileName, err) + log.Error("Unable to live-reload the locale file %q, err: %v", l.sourceFileName, err) } + // We will set the sourceFileInfo to this file to prevent repeated attempts to re-load this broken file + l.sourceFileInfo = sourceFileInfo + l.reloadMu.Unlock() l.reloadMu.RLock() } @@ -225,34 +233,24 @@ func (l *locale) Tr(trKey string, trArgs ...interface{}) string { func (l *locale) tryTr(trKey string, trArgs ...interface{}) (msg string, found bool) { trMsg := trKey - if l.store.reloadMu != nil { - l.store.reloadMu.RLock() - } - idx, ok := l.store.trKeyToIdxMap[trKey] - if l.store.reloadMu != nil { - l.store.reloadMu.RUnlock() - } + // convert the provided trKey to a common idx from the store + idx, ok := l.store.idxForTrKey(trKey) if ok { if msg, found = l.idxToMsgMap[idx]; found { - trMsg = msg // use current translation + trMsg = msg // use the translation that we have found } else if l.langName != l.store.defaultLang { + // No translation available in our current language... fallback to the default language - // attempt to get the default language from the locale store - if l.store.reloadMu != nil { - l.store.reloadMu.RLock() - } - def, ok := l.store.localeMap[l.store.defaultLang] - if l.store.reloadMu != nil { - l.store.reloadMu.RUnlock() - } + // Attempt to get the default language from the locale store + if def, ok := l.store.localeMap[l.store.defaultLang]; ok { - if ok { if def.reloadMu != nil { def.reloadMu.RLock() + def.reloadIfNeeded() } if msg, found = def.idxToMsgMap[idx]; found { - trMsg = msg // use current translation + trMsg = msg // use the translation that we have found } if def.reloadMu != nil { def.reloadMu.RUnlock() @@ -273,8 +271,11 @@ func (l *locale) tryTr(trKey string, trArgs ...interface{}) (msg string, found b for _, arg := range trArgs { val := reflect.ValueOf(arg) if val.Kind() == reflect.Slice { - // before, it can accept Tr(lang, key, a, [b, c], d, [e, f]) as Sprintf(msg, a, b, c, d, e, f), it's an unstable behavior - // now, we restrict the strange behavior and only support: + // Previously, we would accept Tr(lang, key, a, [b, c], d, [e, f]) as Sprintf(msg, a, b, c, d, e, f) + // but this is an unstable behavior. + // + // So we restrict the accepted arguments to either: + // // 1. Tr(lang, key, [slice-items]) as Sprintf(msg, items...) // 2. Tr(lang, key, args...) as Sprintf(msg, args...) if len(trArgs) == 1 { From edb5dd5401a3b1cb2ffc2b69743eb6d3af80a893 Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 29 Jun 2022 20:51:01 +0100 Subject: [PATCH 4/4] Update modules/translation/i18n/i18n.go Co-authored-by: John Olheiser --- modules/translation/i18n/i18n.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/modules/translation/i18n/i18n.go b/modules/translation/i18n/i18n.go index 47a2dc991d640..bb906f3c08c1d 100644 --- a/modules/translation/i18n/i18n.go +++ b/modules/translation/i18n/i18n.go @@ -189,18 +189,16 @@ func (l *locale) reloadIfNeeded() { l.reloadMu.RUnlock() l.reloadMu.Lock() // (NOTE: a pre-emption can occur between these two locks so we need to recheck) + defer l.reloadMu.RLock() + defer l.reloadMu.Unlock() if now.Sub(l.lastReloadCheckTime) < time.Second || l.sourceFileInfo == nil || l.sourceFileName == "" { - l.reloadMu.Unlock() - l.reloadMu.RLock() return } l.lastReloadCheckTime = now sourceFileInfo, err := os.Stat(l.sourceFileName) if err != nil || sourceFileInfo.ModTime().Equal(l.sourceFileInfo.ModTime()) { - l.reloadMu.Unlock() - l.reloadMu.RLock() return } @@ -213,9 +211,6 @@ func (l *locale) reloadIfNeeded() { // We will set the sourceFileInfo to this file to prevent repeated attempts to re-load this broken file l.sourceFileInfo = sourceFileInfo - - l.reloadMu.Unlock() - l.reloadMu.RLock() } // Tr translates content to locale language. fall back to default language.