Add automatic crash detection and restarts#271
Add automatic crash detection and restarts#271michael2847 wants to merge 7 commits intoMinecraftServerControl:mainfrom michael2847:main
Conversation
|
Thanks for the new PR @Roflicide. This looks much better! I had to rebuild my server a month ago due to a failed drive, and haven't yet installed MSCS (I haven't even played Minecraft in what seems like years). I'll try to make some time soon to give this a try. If @zanix or anyone else has the chance to test this out, and approves it before I get the chance, feel free to merge the PR. |
zanix
left a comment
zanix
left a comment
There was a problem hiding this comment.
Can you also make a PR for the documentation to add the new mscs setting options?
michael2847
left a comment
michael2847
left a comment
There was a problem hiding this comment.
hi @zanix, thanks for leaving some feedback. I believe I've fixed all of your requested edits with the exception of the added white space, as I think it's necessary it since the mscs-default-restart-after-crash variable is so long. let me know if you find anything else
thanks!
|
What is the status of this PR? Has anyone been able to test it? I don't have time right now (switching jobs, moving to a new state, etc). The only change request I see is to add documentation for the new options.. Integration with my PR #274 can come before or after this PR as far as I'm concerned.. |
|
I haven't had time to test this, and @Roflicide still need to make a couple changes. |
michael2847
left a comment
michael2847
left a comment
There was a problem hiding this comment.
i think ive made all the changes requested, just gotta update the docs and good to go!
|
Just updated the docs! also, i learned today that anyone else is welcome to do more testing with this PR if they'd like! i'd probably like to do a little more testing, but also good to merge too. |
zanix
left a comment
zanix
left a comment
There was a problem hiding this comment.
Change those calls to flock with $FLOCK
Co-authored-by: Joshua Clark <1580378+zanix@users.noreply.github.com>
Co-authored-by: Joshua Clark <1580378+zanix@users.noreply.github.com>
thanks for the catch, fixed! |
|
Sorry all, I moved across the country and don't currently have my server setup for testing purposes. Has this been tested? Is it now ready to merge? |
|
Hey @Roflicide, this needs a rebase |
sandain
left a comment
sandain
left a comment
There was a problem hiding this comment.
@Roflicide, I left a few minor comments. Once you rebase and fix these remaining minor issues, I'll merge this PR. Thanks for all the work.
| # Nohup does not allow you to pass functions. However, the code below mimics nohup behavior by doing the following: | ||
| # Start subshell, ignore HUP signal, redirect stdin to /dev/null, redirect stdout and stderr to log file, run in background. | ||
| # Also store the PID of this process for later use. | ||
| ( trap "true" HUP ; pid=$(exec sh -c 'echo "$PPID"'); echo $pid > "$MONITOR_PID"; serverMonitor $1 ) </dev/null 2>&1 1>>"$MONITOR_LOG" & |
There was a problem hiding this comment.
Is this "true" value used anywhere? Could this be changed to:
trap "" HUP;
I'm not really familiar with trap, but my reading of the docs leads me to believe that you don't need the "true" value.
Rather than creating the temporary variable $pid, could the second bit be shortened to:
echo $(exec sh -c 'echo "$PPID"') > "$MONITOR_PID";
There was a problem hiding this comment.
Thanks for catching these, shortening them worked!
3 participants
Hi,
This is an updated PR from #257 : New changes:
mscs-enable-restart-after-crashtomscs-restart-after-crash) for brevitymscs-default-restart-after-crashandmscs-restart-after-crash)0/1tofalse/truefor clarity's sakeifstatements when trying to test if the server was running) that made the code less readable, and reverted it back to how the old way was doneI still need to figure out if there's a way to check if there's a server monitor process running without using flock (which the code uses right now). As I mentioned in the last PR, I tried using the
createLockFile()function that's in the script but it didn't work for me for some reason.Probably should also test it a bit more. It does seem to be working as intended on my computer, though!
Thanks