From 0a51dfac9e22b7b29bcbe52c3b0519771f6a07ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Erik=20Pedersen?= Date: Fri, 19 May 2023 12:54:42 +0200 Subject: [PATCH] commands: Fix data race By wrapping all use of the shared config in a lock. Updates #10953 --- commands/commandeer.go | 13 +-- commands/hugobuilder.go | 147 +++++++++++++++++----------- commands/server.go | 210 +++++++++++++++++++++++----------------- 3 files changed, 215 insertions(+), 155 deletions(-) diff --git a/commands/commandeer.go b/commands/commandeer.go index 222a2a020..5fb29d791 100644 --- a/commands/commandeer.go +++ b/commands/commandeer.go @@ -79,18 +79,12 @@ func Execute(args []string) error { } type commonConfig struct { - mu sync.Mutex + mu *sync.Mutex configs *allconfig.Configs cfg config.Provider fs *hugofs.Fs } -func (c *commonConfig) getFs() *hugofs.Fs { - c.mu.Lock() - defer c.mu.Unlock() - return c.fs -} - // This is the root command. type rootCommand struct { Printf func(format string, v ...interface{}) @@ -181,6 +175,7 @@ func (r *rootCommand) ConfigFromConfig(key int32, oldConf *commonConfig) (*commo } return &commonConfig{ + mu: oldConf.mu, configs: configs, cfg: oldConf.cfg, fs: fs, @@ -295,6 +290,7 @@ func (r *rootCommand) ConfigFromProvider(key int32, cfg config.Provider) (*commo } commonConfig := &commonConfig{ + mu: &sync.Mutex{}, configs: configs, cfg: cfg, fs: fs, @@ -309,9 +305,6 @@ func (r *rootCommand) ConfigFromProvider(key int32, cfg config.Provider) (*commo func (r *rootCommand) HugFromConfig(conf *commonConfig) (*hugolib.HugoSites, error) { h, _, err := r.hugoSites.GetOrCreate(r.configVersionID.Load(), func(key int32) (*hugolib.HugoSites, error) { - conf.mu.Lock() - defer conf.mu.Unlock() - depsCfg := deps.DepsCfg{Configs: conf.configs, Fs: conf.fs, Logger: r.logger} return hugolib.NewHugoSites(depsCfg) }) diff --git a/commands/hugobuilder.go b/commands/hugobuilder.go index 4f65140a0..20a3228f9 100644 --- a/commands/hugobuilder.go +++ b/commands/hugobuilder.go @@ -52,8 +52,8 @@ import ( type hugoBuilder struct { r *rootCommand - cunfMu sync.Mutex - conf_ *commonConfig + confmu sync.Mutex + conf *commonConfig // May be nil. s *serverCommand @@ -74,16 +74,17 @@ type hugoBuilder struct { errState hugoBuilderErrState } -func (c *hugoBuilder) conf() *commonConfig { - c.cunfMu.Lock() - defer c.cunfMu.Unlock() - return c.conf_ +func (c *hugoBuilder) withConfE(fn func(conf *commonConfig) error) error { + c.confmu.Lock() + defer c.confmu.Unlock() + return fn(c.conf) } -func (c *hugoBuilder) setConf(conf *commonConfig) { - c.cunfMu.Lock() - defer c.cunfMu.Unlock() - c.conf_ = conf +func (c *hugoBuilder) withConf(fn func(conf *commonConfig)) { + c.confmu.Lock() + defer c.confmu.Unlock() + fn(c.conf) + } type hugoBuilderErrState struct { @@ -357,7 +358,10 @@ func (c *hugoBuilder) newWatcher(pollIntervalStr string, dirList ...string) (*wa // Identifies changes to config (config.toml) files. configSet := make(map[string]bool) - configFiles := c.conf().configs.LoadingInfo.ConfigFiles + var configFiles []string + c.withConf(func(conf *commonConfig) { + configFiles = conf.configs.LoadingInfo.ConfigFiles + }) c.r.logger.Println("Watching for config changes in", strings.Join(configFiles, ", ")) for _, configFile := range configFiles { @@ -466,14 +470,18 @@ func (c *hugoBuilder) copyStaticTo(sourceFs *filesystems.SourceFilesystem) (uint fs := &countingStatFs{Fs: sourceFs.Fs} syncer := fsync.NewSyncer() - syncer.NoTimes = c.conf().configs.Base.NoTimes - syncer.NoChmod = c.conf().configs.Base.NoChmod - syncer.ChmodFilter = chmodFilter + c.withConf(func(conf *commonConfig) { + syncer.NoTimes = conf.configs.Base.NoTimes + syncer.NoChmod = conf.configs.Base.NoChmod + syncer.ChmodFilter = chmodFilter + + syncer.DestFs = conf.fs.PublishDirStatic + // Now that we are using a unionFs for the static directories + // We can effectively clean the publishDir on initial sync + syncer.Delete = conf.configs.Base.CleanDestinationDir + }) + syncer.SrcFs = fs - syncer.DestFs = c.conf().fs.PublishDirStatic - // Now that we are using a unionFs for the static directories - // We can effectively clean the publishDir on initial sync - syncer.Delete = c.conf().configs.Base.CleanDestinationDir if syncer.Delete { c.r.logger.Infoln("removing all files from destination that don't exist in static dirs") @@ -518,9 +526,11 @@ func (c *hugoBuilder) doWithPublishDirs(f func(sourceFs *filesystems.SourceFiles } if lang == "" { // Not multihost - for _, l := range c.conf().configs.Languages { - langCount[l.Lang] = cnt - } + c.withConf(func(conf *commonConfig) { + for _, l := range conf.configs.Languages { + langCount[l.Lang] = cnt + } + }) } else { langCount[lang] = cnt } @@ -562,7 +572,11 @@ func (c *hugoBuilder) fullBuild(noBuildLock bool) error { // Do not copy static files and build sites in parallel if cleanDestinationDir is enabled. // This flag deletes all static resources in /public folder that are missing in /static, // and it does so at the end of copyStatic() call. - if c.conf().configs.Base.CleanDestinationDir { + var cleanDestinationDir bool + c.withConf(func(conf *commonConfig) { + cleanDestinationDir = conf.configs.Base.CleanDestinationDir + }) + if cleanDestinationDir { if err := copyStaticFunc(); err != nil { return err } @@ -692,17 +706,18 @@ func (c *hugoBuilder) handleEvents(watcher *watcher.Batcher, } if ev.Op&fsnotify.Remove == fsnotify.Remove || ev.Op&fsnotify.Rename == fsnotify.Rename { - configFiles := c.conf().configs.LoadingInfo.ConfigFiles - for _, configFile := range configFiles { - counter := 0 - for watcher.Add(configFile) != nil { - counter++ - if counter >= 100 { - break + c.withConf(func(conf *commonConfig) { + for _, configFile := range conf.configs.LoadingInfo.ConfigFiles { + counter := 0 + for watcher.Add(configFile) != nil { + counter++ + if counter >= 100 { + break + } + time.Sleep(100 * time.Millisecond) } - time.Sleep(100 * time.Millisecond) } - } + }) } // Config file(s) changed. Need full rebuild. @@ -834,9 +849,11 @@ func (c *hugoBuilder) handleEvents(watcher *watcher.Batcher, // recursively add new directories to watch list // When mkdir -p is used, only the top directory triggers an event (at least on OSX) if ev.Op&fsnotify.Create == fsnotify.Create { - if s, err := c.conf().fs.Source.Stat(ev.Name); err == nil && s.Mode().IsDir() { - _ = helpers.SymbolicWalk(c.conf().fs.Source, ev.Name, walkAdder) - } + c.withConf(func(conf *commonConfig) { + if s, err := conf.fs.Source.Stat(ev.Name); err == nil && s.Mode().IsDir() { + _ = helpers.SymbolicWalk(conf.fs.Source, ev.Name, walkAdder) + } + }) } if staticSyncer.isStatic(h, ev.Name) { @@ -942,10 +959,16 @@ func (c *hugoBuilder) handleEvents(watcher *watcher.Batcher, } func (c *hugoBuilder) hugo() (*hugolib.HugoSites, error) { - h, err := c.r.HugFromConfig(c.conf()) - if err != nil { + var h *hugolib.HugoSites + if err := c.withConfE(func(conf *commonConfig) error { + var err error + h, err = c.r.HugFromConfig(conf) + return err + + }); err != nil { return nil, err } + if c.s != nil { // A running server, register the media types. for _, s := range h.Sites { @@ -956,7 +979,10 @@ func (c *hugoBuilder) hugo() (*hugolib.HugoSites, error) { } func (c *hugoBuilder) hugoTry() *hugolib.HugoSites { - h, _ := c.r.HugFromConfig(c.conf()) + var h *hugolib.HugoSites + c.withConf(func(conf *commonConfig) { + h, _ = c.r.HugFromConfig(conf) + }) return h } @@ -977,7 +1003,7 @@ func (c *hugoBuilder) loadConfig(cd *simplecobra.Commandeer, running bool) error return err } - c.setConf(conf) + c.conf = conf if c.onConfigLoaded != nil { if err := c.onConfigLoaded(false); err != nil { return err @@ -1014,15 +1040,17 @@ func (c *hugoBuilder) rebuildSites(events []fsnotify.Event) error { return err } if c.fastRenderMode { - // Make sure we always render the home pages - for _, l := range c.conf().configs.Languages { - langPath := h.GetLangSubDir(l.Lang) - if langPath != "" { - langPath = langPath + "/" + c.withConf(func(conf *commonConfig) { + // Make sure we always render the home pages + for _, l := range conf.configs.Languages { + langPath := h.GetLangSubDir(l.Lang) + if langPath != "" { + langPath = langPath + "/" + } + home := h.PrependBasePath("/"+langPath, false) + visited[home] = true } - home := h.PrependBasePath("/"+langPath, false) - visited[home] = true - } + }) } return h.Build(hugolib.BuildCfg{NoBuildLock: true, RecentlyVisited: visited, ErrRecovery: c.errState.wasErr()}, events...) } @@ -1030,18 +1058,25 @@ func (c *hugoBuilder) rebuildSites(events []fsnotify.Event) error { func (c *hugoBuilder) reloadConfig() error { c.r.Reset() c.r.configVersionID.Add(1) - oldConf := c.conf() - conf, err := c.r.ConfigFromConfig(c.r.configVersionID.Load(), c.conf()) - if err != nil { + + if err := c.withConfE(func(conf *commonConfig) error { + oldConf := conf + newConf, err := c.r.ConfigFromConfig(c.r.configVersionID.Load(), conf) + if err != nil { + return err + } + sameLen := len(oldConf.configs.Languages) == len(newConf.configs.Languages) + if !sameLen { + if oldConf.configs.IsMultihost || newConf.configs.IsMultihost { + return errors.New("multihost change detected, please restart server") + } + } + c.conf = newConf + return nil + }); err != nil { return err } - sameLen := len(oldConf.configs.Languages) == len(conf.configs.Languages) - if !sameLen { - if oldConf.configs.IsMultihost || conf.configs.IsMultihost { - return errors.New("multihost change detected, please restart server") - } - } - c.setConf(conf) + if c.onConfigLoaded != nil { if err := c.onConfigLoaded(true); err != nil { return err diff --git a/commands/server.go b/commands/server.go index 4447091be..b42cb1ed3 100644 --- a/commands/server.go +++ b/commands/server.go @@ -44,6 +44,7 @@ import ( "github.com/gohugoio/hugo/common/hugo" "github.com/gohugoio/hugo/common/types" "github.com/gohugoio/hugo/common/urls" + "github.com/gohugoio/hugo/config" "github.com/gohugoio/hugo/helpers" "github.com/gohugoio/hugo/hugofs" "github.com/gohugoio/hugo/hugofs/files" @@ -197,7 +198,6 @@ type fileServer struct { func (f *fileServer) createEndpoint(i int) (*http.ServeMux, net.Listener, string, string, error) { r := f.c.r - conf := f.c.conf() baseURL := f.baseURLs[i] root := f.roots[i] port := f.c.serverPorts[i].p @@ -216,7 +216,11 @@ func (f *fileServer) createEndpoint(i int) (*http.ServeMux, net.Listener, string } } - httpFs := afero.NewHttpFs(conf.fs.PublishDirServer) + var httpFs *afero.HttpFs + f.c.withConf(func(conf *commonConfig) { + httpFs = afero.NewHttpFs(conf.fs.PublishDirServer) + }) + fs := filesOnlyFs{httpFs.Dir(path.Join("/", root))} if i == 0 && f.c.fastRenderMode { r.Println("Running in Fast Render Mode. For full rebuilds on change: hugo server --disableFastRender") @@ -242,9 +246,11 @@ func (f *fileServer) createEndpoint(i int) (*http.ServeMux, net.Listener, string } port = 1313 - if lrport := conf.configs.GetFirstLanguageConfig().BaseURLLiveReload().Port(); lrport != 0 { - port = lrport - } + f.c.withConf(func(conf *commonConfig) { + if lrport := conf.configs.GetFirstLanguageConfig().BaseURLLiveReload().Port(); lrport != 0 { + port = lrport + } + }) lr := *u lr.Host = fmt.Sprintf("%s:%d", lr.Hostname(), port) fmt.Fprint(w, injectLiveReloadScript(r, lr)) @@ -258,7 +264,10 @@ func (f *fileServer) createEndpoint(i int) (*http.ServeMux, net.Listener, string w.Header().Set("Pragma", "no-cache") } - serverConfig := f.c.conf().configs.Base.Server + var serverConfig config.Server + f.c.withConf(func(conf *commonConfig) { + serverConfig = conf.configs.Base.Server + }) // Ignore any query params for the operations below. requestURI, _ := url.PathUnescape(strings.TrimSuffix(r.RequestURI, "?"+r.URL.RawQuery)) @@ -277,7 +286,10 @@ func (f *fileServer) createEndpoint(i int) (*http.ServeMux, net.Listener, string if root != "" { path = filepath.Join(root, path) } - fs := f.c.conf().getFs().PublishDir + var fs afero.Fs + f.c.withConf(func(conf *commonConfig) { + fs = conf.fs.PublishDir + }) fi, err := fs.Stat(path) @@ -519,8 +531,10 @@ func (c *serverCommand) PreRun(cd, runner *simplecobra.Commandeer) error { } if !reloaded && c.fastRenderMode { - c.conf().fs.PublishDir = hugofs.NewHashingFs(c.conf().fs.PublishDir, c.changeDetector) - c.conf().fs.PublishDirStatic = hugofs.NewHashingFs(c.conf().fs.PublishDirStatic, c.changeDetector) + c.withConf(func(conf *commonConfig) { + conf.fs.PublishDir = hugofs.NewHashingFs(conf.fs.PublishDir, c.changeDetector) + conf.fs.PublishDirStatic = hugofs.NewHashingFs(conf.fs.PublishDirStatic, c.changeDetector) + }) } return nil @@ -562,31 +576,33 @@ func (c *serverCommand) setBaseURLsInConfig() error { if len(c.serverPorts) == 0 { panic("no server ports set") } - isMultiHost := c.conf().configs.IsMultihost - for i, language := range c.conf().configs.Languages { - var serverPort int - if isMultiHost { - serverPort = c.serverPorts[i].p - } else { - serverPort = c.serverPorts[0].p - } - langConfig := c.conf().configs.LanguageConfigMap[language.Lang] - baseURLStr, err := c.fixURL(langConfig.BaseURL, c.r.baseURL, serverPort) - if err != nil { - return nil - } - baseURL, err := urls.NewBaseURLFromString(baseURLStr) - if err != nil { - return fmt.Errorf("failed to create baseURL from %q: %s", baseURLStr, err) - } + return c.withConfE(func(conf *commonConfig) error { + for i, language := range conf.configs.Languages { + isMultiHost := conf.configs.IsMultihost + var serverPort int + if isMultiHost { + serverPort = c.serverPorts[i].p + } else { + serverPort = c.serverPorts[0].p + } + langConfig := conf.configs.LanguageConfigMap[language.Lang] + baseURLStr, err := c.fixURL(langConfig.BaseURL, c.r.baseURL, serverPort) + if err != nil { + return err + } + baseURL, err := urls.NewBaseURLFromString(baseURLStr) + if err != nil { + return fmt.Errorf("failed to create baseURL from %q: %s", baseURLStr, err) + } - baseURLLiveReload := baseURL - if c.liveReloadPort != -1 { - baseURLLiveReload, _ = baseURLLiveReload.WithPort(c.liveReloadPort) + baseURLLiveReload := baseURL + if c.liveReloadPort != -1 { + baseURLLiveReload, _ = baseURLLiveReload.WithPort(c.liveReloadPort) + } + langConfig.C.SetBaseURL(baseURL, baseURLLiveReload) } - langConfig.C.SetBaseURL(baseURL, baseURLLiveReload) - } - return nil + return nil + }) } func (c *serverCommand) getErrorWithContext() any { @@ -609,35 +625,42 @@ func (c *serverCommand) getErrorWithContext() any { func (c *serverCommand) createServerPorts(cd *simplecobra.Commandeer) error { flags := cd.CobraCommand.Flags() - isMultiHost := c.conf().configs.IsMultihost - c.serverPorts = make([]serverPortListener, 1) - if isMultiHost { - if !c.serverAppend { - return errors.New("--appendPort=false not supported when in multihost mode") - } - c.serverPorts = make([]serverPortListener, len(c.conf().configs.Languages)) - } - currentServerPort := c.serverPort - for i := 0; i < len(c.serverPorts); i++ { - l, err := net.Listen("tcp", net.JoinHostPort(c.serverInterface, strconv.Itoa(currentServerPort))) - if err == nil { - c.serverPorts[i] = serverPortListener{ln: l, p: currentServerPort} - } else { - if i == 0 && flags.Changed("port") { - // port set explicitly by user -- he/she probably meant it! - return fmt.Errorf("server startup failed: %s", err) + var cerr error + c.withConf(func(conf *commonConfig) { + isMultiHost := conf.configs.IsMultihost + c.serverPorts = make([]serverPortListener, 1) + if isMultiHost { + if !c.serverAppend { + cerr = errors.New("--appendPort=false not supported when in multihost mode") + return } - c.r.Println("port", currentServerPort, "already in use, attempting to use an available port") - l, sp, err := helpers.TCPListen() - if err != nil { - return fmt.Errorf("unable to find alternative port to use: %s", err) - } - c.serverPorts[i] = serverPortListener{ln: l, p: sp.Port} + c.serverPorts = make([]serverPortListener, len(conf.configs.Languages)) } + currentServerPort := c.serverPort + for i := 0; i < len(c.serverPorts); i++ { + l, err := net.Listen("tcp", net.JoinHostPort(c.serverInterface, strconv.Itoa(currentServerPort))) + if err == nil { + c.serverPorts[i] = serverPortListener{ln: l, p: currentServerPort} + } else { + if i == 0 && flags.Changed("port") { + // port set explicitly by user -- he/she probably meant it! + cerr = fmt.Errorf("server startup failed: %s", err) + return + } + c.r.Println("port", currentServerPort, "already in use, attempting to use an available port") + l, sp, err := helpers.TCPListen() + if err != nil { + cerr = fmt.Errorf("unable to find alternative port to use: %s", err) + return + } + c.serverPorts[i] = serverPortListener{ln: l, p: sp.Port} + } - currentServerPort = c.serverPorts[i].p + 1 - } - return nil + currentServerPort = c.serverPorts[i].p + 1 + } + }) + + return cerr } // fixURL massages the baseURL into a form needed for serving @@ -709,28 +732,35 @@ func (c *serverCommand) partialReRender(urls ...string) error { } func (c *serverCommand) serve() error { - isMultiHost := c.conf().configs.IsMultihost - var err error - h, err := c.r.HugFromConfig(c.conf()) - if err != nil { - return err - } - r := c.r - var ( baseURLs []string roots []string + h *hugolib.HugoSites ) - - if isMultiHost { - for _, l := range c.conf().configs.ConfigLangs() { - baseURLs = append(baseURLs, l.BaseURL().String()) - roots = append(roots, l.Language().Lang) + err := c.withConfE(func(conf *commonConfig) error { + isMultiHost := conf.configs.IsMultihost + var err error + h, err = c.r.HugFromConfig(conf) + if err != nil { + return err } - } else { - l := c.conf().configs.GetFirstLanguageConfig() - baseURLs = []string{l.BaseURL().String()} - roots = []string{""} + + if isMultiHost { + for _, l := range conf.configs.ConfigLangs() { + baseURLs = append(baseURLs, l.BaseURL().String()) + roots = append(roots, l.Language().Lang) + } + } else { + l := conf.configs.GetFirstLanguageConfig() + baseURLs = []string{l.BaseURL().String()} + roots = []string{""} + } + + return nil + }) + + if err != nil { + return err } // Cache it here. The HugoSites object may be unavailable later on due to intermittent configuration errors. @@ -796,7 +826,7 @@ func (c *serverCommand) serve() error { mu.HandleFunc(u.Path+"/livereload.js", livereload.ServeJS) mu.HandleFunc(u.Path+"/livereload", livereload.Handler) } - r.Printf("Web Server is available at %s (bind address %s)\n", serverURL, c.serverInterface) + c.r.Printf("Web Server is available at %s (bind address %s)\n", serverURL, c.serverInterface) wg1.Go(func() error { err = srv.Serve(listener) if err != nil && err != http.ErrServerClosed { @@ -829,7 +859,7 @@ func (c *serverCommand) serve() error { } - r.Println("Press Ctrl+C to stop") + c.r.Println("Press Ctrl+C to stop") err = func() error { for { @@ -845,7 +875,7 @@ func (c *serverCommand) serve() error { }() if err != nil { - r.Println("Error:", err) + c.r.Println("Error:", err) } if h := c.hugoTry(); h != nil { @@ -892,17 +922,17 @@ func (s *staticSyncer) syncsStaticEvents(staticEvents []fsnotify.Event) error { publishDir = filepath.Join(publishDir, sourceFs.PublishFolder) } - conf := s.c.conf().configs.Base - fs := s.c.conf().fs syncer := fsync.NewSyncer() - syncer.NoTimes = conf.NoTimes - syncer.NoChmod = conf.NoChmod - syncer.ChmodFilter = chmodFilter - syncer.SrcFs = sourceFs.Fs - syncer.DestFs = fs.PublishDir - if c.s != nil && c.s.renderStaticToDisk { - syncer.DestFs = fs.PublishDirStatic - } + c.withConf(func(conf *commonConfig) { + syncer.NoTimes = conf.configs.Base.NoTimes + syncer.NoChmod = conf.configs.Base.NoChmod + syncer.ChmodFilter = chmodFilter + syncer.SrcFs = sourceFs.Fs + syncer.DestFs = conf.fs.PublishDir + if c.s != nil && c.s.renderStaticToDisk { + syncer.DestFs = conf.fs.PublishDirStatic + } + }) // prevent spamming the log on changes logger := helpers.NewDistinctErrorLogger() @@ -946,7 +976,9 @@ func (s *staticSyncer) syncsStaticEvents(staticEvents []fsnotify.Event) error { if _, err := sourceFs.Fs.Stat(relPath); herrors.IsNotExist(err) { // If file doesn't exist in any static dir, remove it logger.Println("File no longer exists in static dir, removing", relPath) - _ = c.conf().fs.PublishDirStatic.RemoveAll(relPath) + c.withConf(func(conf *commonConfig) { + _ = conf.fs.PublishDirStatic.RemoveAll(relPath) + }) } else if err == nil { // If file still exists, sync it