Skip to content

Commit

Permalink
Enable hugo server SIGINT after loading bad config
Browse files Browse the repository at this point in the history
If a user starts hugo server and writes to the config file in a way
that renders it invalid, hugo server stops responding to SIGINTs,
meaning that a user needs to send a SIGKILL to the hugo process to stop
it.

*commandeer.serve needs to obtain a *hugolib.HugoSites in order to
close the Hugo environment. After handling signals/the stop channel,
*commandeer.serve calls c.hugo() to get the *hugolib.HugoSites. This
function receives from the c.created channel, which is closed when the
HugoSites is ready. However, if an error took place while loading the
config, c.created would never be closed, so Hugo would block on this
channel even after handling SIGINT.

The solution is to close c.created if we failed to load a config when
rebuilding the site, then check at the end of *commandeer.serve whether
the *hugolib.HugoSites is nil. If it is, we stop waiting for a HugoSites
to close, and exit the hugo process.

One issue that resulted from this fix was how to reset the c.created
channel during site rebuilds. In the current code, this is done by
assigning c.commandeerHugoState to a newCommandeerHugoState in
*commandeer.fullRebuild. However, this creates a race condition, since
other goroutines can be reading the value of c.created just as
*commandeer.fullRebuild is attempting to reassign it.

This change fixes the race condition by adding the lazy.Notifier type,
which preserves the use of c.created while allowing for it to be reset
in a goroutine-safe way. c.created is now a lazy.Notifier. I added this
type to the lazy package because it didn't seem worth it to add a new
package, but we can do this if it's cleaner. (I was thinking about using
sync.Cond for this, but the approach I used is closer to the original
use of the c.created channel and changes less code.)

Also adds an integration test for interrupting the server after fixing
an invalid configuration. An earlier version of this change caused hugo
server to stop exiting with a SIGINT after fixing a bad config, so I
wanted to ensure that we can prevent this.

Fixes gohugoio#8340
  • Loading branch information
ptgott committed Mar 13, 2022
1 parent 38f778c commit ea64569
Show file tree
Hide file tree
Showing 6 changed files with 340 additions and 8 deletions.
12 changes: 6 additions & 6 deletions commands/commandeer.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"time"

hconfig "github.com/gohugoio/hugo/config"
"github.com/gohugoio/hugo/lazy"

"golang.org/x/sync/semaphore"

Expand Down Expand Up @@ -52,7 +53,7 @@ type commandeerHugoState struct {
*deps.DepsCfg
hugoSites *hugolib.HugoSites
fsCreate sync.Once
created chan struct{}
created *lazy.Notifier
}

type commandeer struct {
Expand Down Expand Up @@ -107,12 +108,12 @@ type commandeer struct {

func newCommandeerHugoState() *commandeerHugoState {
return &commandeerHugoState{
created: make(chan struct{}),
created: lazy.NewNotifier(),
}
}

func (c *commandeerHugoState) hugo() *hugolib.HugoSites {
<-c.created
c.created.Wait()
return c.hugoSites
}

Expand Down Expand Up @@ -359,7 +360,6 @@ func (c *commandeer) loadConfig() error {
if err != nil {
return err
}

cfg.Logger = logger
c.logger = logger
c.serverConfig, err = hconfig.DecodeServer(cfg.Cfg)
Expand Down Expand Up @@ -409,7 +409,7 @@ func (c *commandeer) loadConfig() error {

err = c.initFs(fs)
if err != nil {
close(c.created)
c.created.Close()
return
}

Expand All @@ -425,7 +425,7 @@ func (c *commandeer) loadConfig() error {
if c.buildLock == nil && h != nil {
c.buildLock = h.LockBuild
}
close(c.created)
c.created.Close()
})

if err != nil {
Expand Down
8 changes: 7 additions & 1 deletion commands/hugo.go
Original file line number Diff line number Diff line change
Expand Up @@ -773,6 +773,7 @@ func (c *commandeer) partialReRender(urls ...string) error {
}

func (c *commandeer) fullRebuild(changeType string) {
c.created.Reset()
if changeType == configChangeGoMod {
// go.mod may be changed during the build itself, and
// we really want to prevent superfluous builds.
Expand All @@ -798,15 +799,16 @@ func (c *commandeer) fullRebuild(changeType string) {

defer c.timeTrack(time.Now(), "Rebuilt")

c.commandeerHugoState = newCommandeerHugoState()
err := c.loadConfig()
if err != nil {
// Set the processing on pause until the state is recovered.
c.paused = true
c.created.Close()
c.handleBuildErr(err, "Failed to reload config")

} else {
c.paused = false
c.created.Close()
}

if !c.paused {
Expand Down Expand Up @@ -881,6 +883,10 @@ func (c *commandeer) newWatcher(pollIntervalStr string, dirList ...string) (*wat
return
}
c.handleEvents(watcher, staticSyncer, evs, configSet)
// We might be reloading the configuration in a separate
// goroutine, so make sure the config is ready before we
// read from it.
c.created.Wait()
if c.showErrorInBrowser && c.errCount() > 0 {
// Need to reload browser to show the error
livereload.ForceRefresh()
Expand Down
6 changes: 5 additions & 1 deletion commands/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,11 @@ func (c *commandeer) serve(s *serverCmd) error {
<-sigs
}

c.hugo().Close()
// h will be nil if there was an error preparing the Hugo environment,
// e.g., if we couldn't load the config.
if h := c.hugo(); h != nil {
h.Close()
}

return nil
}
Expand Down
171 changes: 171 additions & 0 deletions commands/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@ import (
"fmt"
"net/http"
"os"
"path/filepath"
"runtime"
"strings"
"testing"
"time"

"github.com/gohugoio/hugo/config"
"github.com/gohugoio/hugo/helpers"
"github.com/gohugoio/hugo/htesting"
"github.com/gohugoio/hugo/hugofs"

qt "github.com/frankban/quicktest"
)
Expand Down Expand Up @@ -102,6 +105,174 @@ func runServerTestAndGetHome(c *qt.C, config string) (string, error) {
return homeContent, nil
}

// Should be able to interrupt the hugo server command after the server detects
// a configuration change and the configuration is malformed.
// Issue 8340
func TestInterruptAfterBadConfig(t *testing.T) {
// Test failure takes the form of a timeout here, so ensure there's always
// a timeout, and that it isn't overly long.
bail := time.After(time.Duration(10) * time.Second)

c := qt.New(t)
dir, clean, err := htesting.CreateTempDir(hugofs.Os, "hugo-cli")
defer clean()

cfgStr := `
baseURL = "https://example.org"
title = "Hugo Commands"
`
os.MkdirAll(filepath.Join(dir, "public"), 0777)
os.MkdirAll(filepath.Join(dir, "data"), 0777)
os.MkdirAll(filepath.Join(dir, "layouts"), 0777)

writeFile(t, filepath.Join(dir, "config.toml"), cfgStr)
writeFile(t, filepath.Join(dir, "content", "p1.md"), `
---
title: "P1"
weight: 1
---
Content
`)
c.Assert(err, qt.IsNil)

port := 1331

b := newCommandsBuilder()
stop := make(chan bool)
done := make(chan struct{})
scmd := b.newServerCmdSignaled(stop)

cmd := scmd.getCommand()
cmd.SetArgs([]string{
"-s=" + dir,
fmt.Sprintf("-p=%d", port),
"-d=" + filepath.Join(dir, "public"),
})

go func(n chan struct{}) {
_, err = cmd.ExecuteC()
c.Assert(err, qt.IsNil)
done <- struct{}{}
}(done)

// Wait for the server to be ready
time.Sleep(2 * time.Second)

// Break the config file
writeFile(t, filepath.Join(dir, "config.toml"), `
baseURL = "https://example.org"
title = "Hugo Commands"
theme = "notarealtheme
`)

// Wait for the server to make the change
time.Sleep(2 * time.Second)

go func() {
// don't block on stopping the server
stop <- true
}()

select {
case <-done:
return
case <-bail:
t.Fatal("test timed out waiting for the server to stop")
}

}

func TestFixBadConfig(t *testing.T) {
// Test failure takes the form of a timeout here, so ensure there's always
// a timeout, and that it isn't overly long.
bail := time.After(time.Duration(10) * time.Second)
c := qt.New(t)
dir, clean, err := htesting.CreateTempDir(hugofs.Os, "hugo-cli")
defer clean()

cfgStr := `
baseURL = "https://example.org"
title = "Hugo Commands"
`
os.MkdirAll(filepath.Join(dir, "public"), 0777)
os.MkdirAll(filepath.Join(dir, "data"), 0777)
os.MkdirAll(filepath.Join(dir, "layouts"), 0777)

writeFile(t, filepath.Join(dir, "config.toml"), cfgStr)
writeFile(t, filepath.Join(dir, "content", "p1.md"), `
---
title: "P1"
weight: 1
---
Content
`)
c.Assert(err, qt.IsNil)

port := 1331

b := newCommandsBuilder()
b.logging = true
stop := make(chan bool)
done := make(chan struct{})
scmd := b.newServerCmdSignaled(stop)

cmd := scmd.getCommand()
cmd.SetArgs([]string{
"-s=" + dir,
fmt.Sprintf("-p=%d", port),
"-d=" + filepath.Join(dir, "public"),
})

go func(n chan struct{}) {
_, err = cmd.ExecuteC()
c.Assert(err, qt.IsNil)
done <- struct{}{}
}(done)

// Wait for the server to be ready
time.Sleep(2 * time.Second)

// Break the config file
writeFile(t, filepath.Join(dir, "config.toml"), `
baseURL = "https://example.org"
title = "Hugo Commands"
theme = "notarealtheme
`)

// Wait for the server to make the change
time.Sleep(2 * time.Second)

// Fix the config file
writeFile(t, filepath.Join(dir, "config.toml"), cfgStr)

// Wait for the FS watcher to respond
time.Sleep(2 * time.Second)

go func() {
// don't block on stopping the server
stop <- true
}()

select {
case <-done:
return
case <-bail:
t.Fatal("test timed out waiting for the server to stop")
}
}

func TestFixURL(t *testing.T) {
type data struct {
TestName string
Expand Down
99 changes: 99 additions & 0 deletions lazy/notifier.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// Copyright 2019 The Hugo Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package lazy

import (
"runtime/debug"
"strings"
"sync"
)

// Notifier as a synchronization tool that is queried for just-in-time access
// to a resource. Callers use Wait to block until the resource is ready, and
// call Close to indicate that the resource is ready. Reset returns the
// resource to its locked state.
//
// Notifier must be initialized by calling NewNotifier.
type Notifier struct {
// Channel to close when the protected resource is ready. This must only
// be accessed via calling the currentCh method to avoid race conditions.
ch chan struct{}
// For locking the channel while resetting it
mu *sync.RWMutex
}

// NewNotifier creates a Notifier with all synchronization mechanisms
// initialized.
func NewNotifier() *Notifier {
return &Notifier{
ch: make(chan struct{}),
mu: &sync.RWMutex{},
}
}

// isClosed checks whether a channel is closed. If calling from a Notifier
// method, the calling goroutine must hold and release the Notifier.mu lock.
func isClosed(ch chan struct{}) bool {
select {
// Already closed
case <-ch:
return true
default:
return false
}
}

// currentCh safely returns the current channel. Because this locks and unlocks
// the mutex, callers must not perform any other locking until the channel is
// returned.
func (n *Notifier) currentCh() chan struct{} {
n.mu.RLock()
defer n.mu.RUnlock()
return n.ch
}

// Wait waits for the Notifier to be ready, i.e., for Close to be called
// somewhere
func (n *Notifier) Wait() {
ch := n.currentCh()
<-ch
s := string(debug.Stack())
if strings.Contains(s, "newWatcher") {
}
return
}

// Close unblocks any goroutines that called Wait
func (n *Notifier) Close() {
ch := n.currentCh()
n.mu.Lock()
defer n.mu.Unlock()
if !isClosed(ch) {
close(ch)
}
return
}

// Reset returns the resource to its pre-ready state while locking
func (n *Notifier) Reset() {
ch := n.currentCh()
n.mu.Lock()
// No need to reset since the channel is open
if !isClosed(ch) {
return
}
defer n.mu.Unlock()
n.ch = make(chan struct{})
return
}
Loading

0 comments on commit ea64569

Please sign in to comment.