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 <[email protected]>

Author: zeripath

modules/translation/i18n/i18n.go

@@ -25,174 +25,268 @@ 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
 	lastReloadCheckTime time.Time
 }
 
 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