Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable hugo server SIGINT after loading bad config #9664

Closed

Conversation

ptgott
Copy link
Contributor

@ptgott ptgott commented Mar 13, 2022

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.)

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 #8340

@ptgott ptgott force-pushed the 8340-ctrl-c-broken-config-no-race branch 3 times, most recently from 1693500 to ef85907 Compare March 18, 2022 13:07
@bep
Copy link
Member

bep commented Mar 18, 2022

Thanks for this; I'm sure what you have implemented works, but looking at your description I think we need to take a step back and solve this by reducing some of the complexity (which I'm certainly to blame for, but I got some of it from the starting point which was lots of globals) not add more complexity.

I'm not sure what that would be, but I'll think about it. I'll leave this PR open for if I can not come up with something simpler.

@ptgott
Copy link
Contributor Author

ptgott commented Mar 20, 2022

👍 I'll keep this rebased in the meantime just in case.

@ptgott ptgott force-pushed the 8340-ctrl-c-broken-config-no-race branch 2 times, most recently from 98bbbc4 to aad2846 Compare March 26, 2022 21:10
@ptgott ptgott force-pushed the 8340-ctrl-c-broken-config-no-race branch from aad2846 to 2138359 Compare April 10, 2022 17:45
@ptgott ptgott force-pushed the 8340-ctrl-c-broken-config-no-race branch from 2138359 to bd79bad Compare April 23, 2022 18:39
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.)

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
@ptgott ptgott force-pushed the 8340-ctrl-c-broken-config-no-race branch from bd79bad to 0666e43 Compare May 14, 2022 22:06
bep added a commit to bep/hugo that referenced this pull request May 15, 2022
@bep bep closed this in #9899 May 15, 2022
bep added a commit that referenced this pull request May 15, 2022
Also fix the config error messages.

Fixes #9664
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hugo server doesn't stop on Ctrl+C if theme name is misspelled after starting the server
2 participants