serve: Ensure redis-server saves after signal#138
Merged
Conversation
This is part one of improved shutdown handling; part two will come in a future commit involving a more graceful shutdown of the main node process itself. I began to notice that running `./go serve` to launch the redis-server, using the app, and then killing the script quickly resulted in data loss. This didn't happen when running redis-server manually, as a signal sent to that process would result in it saving its data before exiting. The problem appeared to be that signals sent to the `./go serve` script would be sent simultaneously to the redis-server process, under which circumstances it wouldn't save its data automatically. The script now launches redis-server in a separate process group so that any signals sent to the script aren't sent to the redis-server process. The custom-links server itself now runs as a background process so that the it can be managed explicitly via a trap on the TERM, INT, and HUP signals. Finally, an EXIT trap sends the `shutdown save` command to the redis-server and waits for the process to exit to complete a graceful shutdown with all data saved. The redis-server also now runs with `--appendonly yes` specified, providing more durability against data loss. This also marks the first time Bats tests have been added for any of the scripts. These are now included as part of `./go test`. The `./go serve` script is pretty well-tested at this point, but a few notes: * `scripts/lib/config-json` needs tests specific to it * The new helper functions added to `tests/scripts/serve.bats`, namely `run_in_background`, `wait_for_background_output`, and `stop_background_run`, should get moved into the `go-script-bash` framework. * `scripts/lib/config-json` and parts of `scripts/serve` might make sense to move into `go-script-bash` as well. * Coverage for `./go test scripts` isn't yet enabled.
|
Coverage remained the same at 98.089% when pulling cb848871daedd40c78666d805d68d201cc727cff on save-on-exit into 38ab7f3 on master. |
I want to do a PR specifically focusing on enabling coverage for Bats tests, and to see if it can be unified with the JavaScript coverage reports.
1 similar comment
Turns out `pgrep -q` isn't available on Linux.
This appeared to be causing failues on Travis with colorized output. Also fixes the tab completion test, since `test-config.json` doesn't exist out of the box.
On Linux, `nc` will exit with an error if the hostname argument is empty. Setting it to `localhost` by default fixes the problem. On Travis, it appears that file descriptors for standard output and error still look like terminal file descriptors to Bash, even after being redirected to a file (i.e. `[[ -t 0 && -t 1 ]]` is always true)`. This causes the `log` module from `go-script-bash` to always emit terminal-formatted log labels. This played havoc with the previous `assert_output_matches` assertions, which only checked for whitespace between log label text and the log message text. The fix was to update each assertion so that all log labels are immediately followed by `.* `.
This was referenced Aug 26, 2017
Open
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is part one of improved shutdown handling; part two will come in a future commit involving a more graceful shutdown of the main node process itself.
I began to notice that running
./go serveto launch the redis-server, using the app, and then killing the script quickly resulted in data loss. This didn't happen when running redis-server manually, as a signal sent to that process would result in it saving its data before exiting. The problem appeared to be that signals sent to the./go servescript would be sent simultaneously to the redis-server process, under which circumstances it wouldn't save its data automatically.The script now launches redis-server in a separate process group so that any signals sent to the script aren't sent to the redis-server process. The custom-links server itself now runs as a background process so that the it can be managed explicitly via a trap on the TERM, INT, and HUP signals. Finally, an EXIT trap sends the
shutdown savecommand to the redis-server and waits for the process to exit to complete a graceful shutdown with all data saved.The redis-server also now runs with
--appendonly yesspecified, providing more durability against data loss.This also marks the first time Bats tests have been added for any of the scripts. These are now included as part of
./go test.The
./go servescript is pretty well-tested at this point, but a few notes:scripts/lib/config-jsonneeds tests specific to ittests/scripts/serve.bats, namelyrun_in_background,wait_for_background_output, andstop_background_run, should get moved into thego-script-bashframework.scripts/lib/config-jsonand parts ofscripts/servemight make sense to move intogo-script-bashas well../go test scriptsisn't yet enabled.