From ba0f9274e99ce7e28d587e30eee4a211b0556354 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 4 Jul 2022 11:17:09 +0100 Subject: [PATCH] Allow dev i18n to be more concurrent (#20159) The recent changes to add live-reloading to the i18n translation files made the i18n code totally non-concurrent when using dev. This will make discovering other concurrency related issues far more difficult. 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, 168 insertions(+), 74 deletions(-) diff --git a/modules/translation/i18n/i18n.go b/modules/translation/i18n/i18n.go index acce5f19f..bb906f3c0 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 trKeyToIdxMap sourceFileName string sourceFileInfo os.FileInfo @@ -35,164 +39,254 @@ 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 - localeMap map[string]*locale - textIdxMap map[string]int + // this needs to be locked when live-reloading + 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 { +// Note: this is not concurrent safe +func (store *LocaleStore) AddLocaleByIni(langName, langDesc string, source interface{}) error { + 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() // Arguably this is not necessary as AddLocaleByIni isn't concurrent safe - but for consistency we do this + 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 + } + + var err error + l.idxToMsgMap, err = store.readIniToIdxToMsgMap(source) + if err != nil { + return err } - 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 nil } -func (ls *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 - lc := ls.localeMap[langName] - lc.textMap = 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() { + 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() + idxToMsgMap[idx] = key.Value() } } iniFile = nil - return nil + return idxToMsgMap, nil +} + +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 } -func (ls *LocaleStore) HasLang(langName string) bool { - _, ok := ls.localeMap[langName] +// HasLang reports if a language is available in the store +func (store *LocaleStore) HasLang(langName string) bool { + _, ok := store.localeMap[langName] return ok } -func (ls *LocaleStore) ListLangNameDesc() (names, desc []string) { - return ls.langNames, ls.langDescs +// ListLangNameDesc reports if a language available in the store +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 { + l, ok := store.localeMap[lang] if !ok { - l, ok = ls.localeMap[ls.defaultLang] + l, ok = store.localeMap[store.defaultLang] } + 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.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) + defer l.reloadMu.RLock() + defer l.reloadMu.Unlock() + + if now.Sub(l.lastReloadCheckTime) < time.Second || l.sourceFileInfo == nil || l.sourceFileName == "" { + return + } + + l.lastReloadCheckTime = now + sourceFileInfo, err := os.Stat(l.sourceFileName) + if err != nil || sourceFileInfo.ModTime().Equal(l.sourceFileInfo.ModTime()) { + return + } + + idxToMsgMap, err := l.store.readIniToIdxToMsgMap(l.sourceFileName) + if err == nil { + l.idxToMsgMap = idxToMsgMap + } else { + 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 +} + // 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] + + // convert the provided trKey to a common idx from the store + idx, ok := l.store.idxForTrKey(trKey) + if ok { - if msg, found = l.textMap[textIdx]; found { - trMsg = msg // use current translation + if msg, found = l.idxToMsgMap[idx]; found { + 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 def, ok := l.store.localeMap[l.store.defaultLang]; ok { - return def.tryTr(trKey, trArgs...) + + if def.reloadMu != nil { + def.reloadMu.RLock() + def.reloadIfNeeded() + } + if msg, found = def.idxToMsgMap[idx]; found { + trMsg = msg // use the translation that we have found + } + 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 { + // 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 { + 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