* [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload.
@ 2017-09-25 13:20 Thomas Claveirole
2017-09-26 10:00 ` Johan Oudinet
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Thomas Claveirole @ 2017-09-25 13:20 UTC (permalink / raw)
To: buildroot
Fix package/nginx/S50nginx:
* On stop, use start-stop-daemon -R 1 to wait for the nginx processes
to actually stop. This fixes a race condition with restart, where
nginx fails to restart because start is called too early
w.r.t. stop. (This only works with Debian's start-stop-daemon,
however BusyBox's start-stop-daemon does not fail when given -R; it
just ignores the argument silently).
* Implement reload with an actual reload instead of a restart.
* Add force-reload.
Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
---
package/nginx/S50nginx | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/package/nginx/S50nginx b/package/nginx/S50nginx
index b2f8b80855..a854c651d1 100755
--- a/package/nginx/S50nginx
+++ b/package/nginx/S50nginx
@@ -3,23 +3,31 @@
# Start/stop nginx
#
+NGINX=/usr/sbin/nginx
PIDFILE=/var/run/nginx.pid
case "$1" in
start)
echo "Starting nginx..."
mkdir -p /var/log/nginx /var/tmp/nginx
- start-stop-daemon -S -x /usr/sbin/nginx -p $PIDFILE
+ start-stop-daemon -S -x "$NGINX" -p "$PIDFILE"
;;
stop)
- printf "Stopping nginx..."
- start-stop-daemon -K -o -p $PIDFILE
+ echo "Stopping nginx..."
+ # Use -R 1 to wait for nginx to actually stop. Useful so
+ # restart has no race condition. Note that BusyBox knows
+ # about -R but ignores it silently.
+ start-stop-daemon -K -x "$NGINX" -p "$PIDFILE" -R 1 -o
;;
- restart|reload)
+ reload|force-reload)
+ echo "Reloading nginx configuration..."
+ "$NGINX" -s reload
+ ;;
+ restart)
"$0" stop
"$0" start
;;
*)
- echo "Usage: $0 {start|stop|restart}"
+ echo "Usage: $0 {start|stop|restart|reload|force-reload}"
exit 1
esac
--
2.14.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload.
2017-09-25 13:20 [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Thomas Claveirole
@ 2017-09-26 10:00 ` Johan Oudinet
2017-09-26 20:05 ` Samuel Martin
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Johan Oudinet @ 2017-09-26 10:00 UTC (permalink / raw)
To: buildroot
Hello,
On Mon, Sep 25, 2017 at 3:20 PM, Thomas Claveirole
<thomas.claveirole@green-communications.fr> wrote:
> Fix package/nginx/S50nginx:
>
> * On stop, use start-stop-daemon -R 1 to wait for the nginx processes
> to actually stop. This fixes a race condition with restart, where
> nginx fails to restart because start is called too early
> w.r.t. stop. (This only works with Debian's start-stop-daemon,
> however BusyBox's start-stop-daemon does not fail when given -R; it
> just ignores the argument silently).
>
> * Implement reload with an actual reload instead of a restart.
>
> * Add force-reload.
>
> Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
> ---
> package/nginx/S50nginx | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/package/nginx/S50nginx b/package/nginx/S50nginx
> index b2f8b80855..a854c651d1 100755
> --- a/package/nginx/S50nginx
> +++ b/package/nginx/S50nginx
> @@ -3,23 +3,31 @@
> # Start/stop nginx
> #
>
> +NGINX=/usr/sbin/nginx
> PIDFILE=/var/run/nginx.pid
>
> case "$1" in
> start)
> echo "Starting nginx..."
> mkdir -p /var/log/nginx /var/tmp/nginx
> - start-stop-daemon -S -x /usr/sbin/nginx -p $PIDFILE
> + start-stop-daemon -S -x "$NGINX" -p "$PIDFILE"
> ;;
> stop)
> - printf "Stopping nginx..."
> - start-stop-daemon -K -o -p $PIDFILE
> + echo "Stopping nginx..."
> + # Use -R 1 to wait for nginx to actually stop. Useful so
> + # restart has no race condition. Note that BusyBox knows
> + # about -R but ignores it silently.
> + start-stop-daemon -K -x "$NGINX" -p "$PIDFILE" -R 1 -o
> ;;
> - restart|reload)
> + reload|force-reload)
> + echo "Reloading nginx configuration..."
> + "$NGINX" -s reload
> + ;;
> + restart)
> "$0" stop
> "$0" start
> ;;
> *)
> - echo "Usage: $0 {start|stop|restart}"
> + echo "Usage: $0 {start|stop|restart|reload|force-reload}"
> exit 1
> esac
> --
> 2.14.1
I've added the maintainer of nginx (Samuel Martin) in CC, as reported
by get-developers.
Otherwise, this patch looks good to me.
Best,
--
Johan
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload.
2017-09-25 13:20 [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Thomas Claveirole
2017-09-26 10:00 ` Johan Oudinet
@ 2017-09-26 20:05 ` Samuel Martin
2017-09-26 22:39 ` Arnout Vandecappelle
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Samuel Martin @ 2017-09-26 20:05 UTC (permalink / raw)
To: buildroot
On Mon, Sep 25, 2017 at 3:20 PM, Thomas Claveirole
<thomas.claveirole@green-communications.fr> wrote:
> Fix package/nginx/S50nginx:
>
> * On stop, use start-stop-daemon -R 1 to wait for the nginx processes
> to actually stop. This fixes a race condition with restart, where
> nginx fails to restart because start is called too early
> w.r.t. stop. (This only works with Debian's start-stop-daemon,
> however BusyBox's start-stop-daemon does not fail when given -R; it
> just ignores the argument silently).
>
> * Implement reload with an actual reload instead of a restart.
>
> * Add force-reload.
>
> Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
Reviewed-by: Samuel Martin <s.martin49@gmail.com>
Regards,
--
Samuel
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload.
2017-09-25 13:20 [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Thomas Claveirole
2017-09-26 10:00 ` Johan Oudinet
2017-09-26 20:05 ` Samuel Martin
@ 2017-09-26 22:39 ` Arnout Vandecappelle
2017-09-28 19:31 ` Peter Korsgaard
2017-10-17 8:58 ` [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Peter Korsgaard
4 siblings, 0 replies; 9+ messages in thread
From: Arnout Vandecappelle @ 2017-09-26 22:39 UTC (permalink / raw)
To: buildroot
On 25-09-17 15:20, Thomas Claveirole wrote:
> Fix package/nginx/S50nginx:
>
> * On stop, use start-stop-daemon -R 1 to wait for the nginx processes
> to actually stop. This fixes a race condition with restart, where
> nginx fails to restart because start is called too early
> w.r.t. stop. (This only works with Debian's start-stop-daemon,
> however BusyBox's start-stop-daemon does not fail when given -R; it
> just ignores the argument silently).
>
> * Implement reload with an actual reload instead of a restart.
>
> * Add force-reload.
>
> Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
Applied to master, thanks.
Regards,
Arnout
> ---
> package/nginx/S50nginx | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/package/nginx/S50nginx b/package/nginx/S50nginx
> index b2f8b80855..a854c651d1 100755
> --- a/package/nginx/S50nginx
> +++ b/package/nginx/S50nginx
> @@ -3,23 +3,31 @@
> # Start/stop nginx
> #
>
> +NGINX=/usr/sbin/nginx
> PIDFILE=/var/run/nginx.pid
>
> case "$1" in
> start)
> echo "Starting nginx..."
> mkdir -p /var/log/nginx /var/tmp/nginx
> - start-stop-daemon -S -x /usr/sbin/nginx -p $PIDFILE
> + start-stop-daemon -S -x "$NGINX" -p "$PIDFILE"
> ;;
> stop)
> - printf "Stopping nginx..."
> - start-stop-daemon -K -o -p $PIDFILE
> + echo "Stopping nginx..."
> + # Use -R 1 to wait for nginx to actually stop. Useful so
> + # restart has no race condition. Note that BusyBox knows
> + # about -R but ignores it silently.
> + start-stop-daemon -K -x "$NGINX" -p "$PIDFILE" -R 1 -o
> ;;
> - restart|reload)
> + reload|force-reload)
> + echo "Reloading nginx configuration..."
> + "$NGINX" -s reload
> + ;;
> + restart)
> "$0" stop
> "$0" start
> ;;
> *)
> - echo "Usage: $0 {start|stop|restart}"
> + echo "Usage: $0 {start|stop|restart|reload|force-reload}"
> exit 1
> esac
>
--
Arnout Vandecappelle arnout at mind be
Senior Embedded Software Architect +32-16-286500
Essensium/Mind http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload.
2017-09-25 13:20 [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Thomas Claveirole
` (2 preceding siblings ...)
2017-09-26 22:39 ` Arnout Vandecappelle
@ 2017-09-28 19:31 ` Peter Korsgaard
2017-10-09 9:50 ` [Buildroot] [PATCH 1/1] package/nginx/S50nginx: Do not assume start-stop-daemon knows -R Thomas Claveirole
2017-10-17 8:58 ` [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Peter Korsgaard
4 siblings, 1 reply; 9+ messages in thread
From: Peter Korsgaard @ 2017-09-28 19:31 UTC (permalink / raw)
To: buildroot
>>>>> "Thomas" == Thomas Claveirole <thomas.claveirole@green-communications.fr> writes:
> Fix package/nginx/S50nginx:
> * On stop, use start-stop-daemon -R 1 to wait for the nginx processes
> to actually stop. This fixes a race condition with restart, where
> nginx fails to restart because start is called too early
> w.r.t. stop. (This only works with Debian's start-stop-daemon,
> however BusyBox's start-stop-daemon does not fail when given -R; it
> just ignores the argument silently).
> * Implement reload with an actual reload instead of a restart.
> * Add force-reload.
> Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
> ---
> package/nginx/S50nginx | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
> diff --git a/package/nginx/S50nginx b/package/nginx/S50nginx
> index b2f8b80855..a854c651d1 100755
> --- a/package/nginx/S50nginx
> +++ b/package/nginx/S50nginx
> @@ -3,23 +3,31 @@
> # Start/stop nginx
> #
> +NGINX=/usr/sbin/nginx
> PIDFILE=/var/run/nginx.pid
> case "$1" in
> start)
> echo "Starting nginx..."
> mkdir -p /var/log/nginx /var/tmp/nginx
> - start-stop-daemon -S -x /usr/sbin/nginx -p $PIDFILE
> + start-stop-daemon -S -x "$NGINX" -p "$PIDFILE"
> ;;
> stop)
> - printf "Stopping nginx..."
> - start-stop-daemon -K -o -p $PIDFILE
> + echo "Stopping nginx..."
> + # Use -R 1 to wait for nginx to actually stop. Useful so
> + # restart has no race condition. Note that BusyBox knows
> + # about -R but ignores it silently.
> + start-stop-daemon -K -x "$NGINX" -p "$PIDFILE" -R 1 -o
So it only fixes something if the "big" start-stop-daemon is used. Even
worse, if breaks it for busybox users that don't have
CONFIG_FEATURE_START_STOP_DAEMON_FANCY enabled:
start-stop-daemon: invalid option -- 'R'
Perhaps a more pragmatic solution would be to simply add a sleep 1
between stop and start in the restart case?
> ;;
> - restart|reload)
> + reload|force-reload)
> + echo "Reloading nginx configuration..."
> + "$NGINX" -s reload
> + ;;
> + restart)
> "$0" stop
> "$0" start
> ;;
> *)
> - echo "Usage: $0 {start|stop|restart}"
> + echo "Usage: $0 {start|stop|restart|reload|force-reload}"
> exit 1
> esac
> --
> 2.14.1
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/nginx/S50nginx: Do not assume start-stop-daemon knows -R.
2017-09-28 19:31 ` Peter Korsgaard
@ 2017-10-09 9:50 ` Thomas Claveirole
2017-10-10 15:54 ` Peter Korsgaard
2017-10-17 9:07 ` Peter Korsgaard
0 siblings, 2 replies; 9+ messages in thread
From: Thomas Claveirole @ 2017-10-09 9:50 UTC (permalink / raw)
To: buildroot
start-stop-daemon fails on -R when not compiled with
CONFIG_FEATURE_START_STOP_DAEMON_FANCY. Thus, do not rely on -R
during stop to avoid a race condition during restart.
Use a sleep 1 during restart instead, as suggested by Peter Korsgaard
in <87bmluk4bm.fsf@dell.be.48ers.dk>.
Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
---
package/nginx/S50nginx | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/package/nginx/S50nginx b/package/nginx/S50nginx
index a854c651d1..964652b922 100755
--- a/package/nginx/S50nginx
+++ b/package/nginx/S50nginx
@@ -14,10 +14,7 @@ case "$1" in
;;
stop)
echo "Stopping nginx..."
- # Use -R 1 to wait for nginx to actually stop. Useful so
- # restart has no race condition. Note that BusyBox knows
- # about -R but ignores it silently.
- start-stop-daemon -K -x "$NGINX" -p "$PIDFILE" -R 1 -o
+ start-stop-daemon -K -x "$NGINX" -p "$PIDFILE" -o
;;
reload|force-reload)
echo "Reloading nginx configuration..."
@@ -25,6 +22,7 @@ case "$1" in
;;
restart)
"$0" stop
+ sleep 1 # Prevent race condition: ensure nginx stops before start.
"$0" start
;;
*)
--
2.14.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/nginx/S50nginx: Do not assume start-stop-daemon knows -R.
2017-10-09 9:50 ` [Buildroot] [PATCH 1/1] package/nginx/S50nginx: Do not assume start-stop-daemon knows -R Thomas Claveirole
@ 2017-10-10 15:54 ` Peter Korsgaard
2017-10-17 9:07 ` Peter Korsgaard
1 sibling, 0 replies; 9+ messages in thread
From: Peter Korsgaard @ 2017-10-10 15:54 UTC (permalink / raw)
To: buildroot
>>>>> "Thomas" == Thomas Claveirole <thomas.claveirole@green-communications.fr> writes:
> start-stop-daemon fails on -R when not compiled with
> CONFIG_FEATURE_START_STOP_DAEMON_FANCY. Thus, do not rely on -R
> during stop to avoid a race condition during restart.
> Use a sleep 1 during restart instead, as suggested by Peter Korsgaard
> in <87bmluk4bm.fsf@dell.be.48ers.dk>.
> Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
Committed, thanks.
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload.
2017-09-25 13:20 [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Thomas Claveirole
` (3 preceding siblings ...)
2017-09-28 19:31 ` Peter Korsgaard
@ 2017-10-17 8:58 ` Peter Korsgaard
4 siblings, 0 replies; 9+ messages in thread
From: Peter Korsgaard @ 2017-10-17 8:58 UTC (permalink / raw)
To: buildroot
>>>>> "Thomas" == Thomas Claveirole <thomas.claveirole@green-communications.fr> writes:
> Fix package/nginx/S50nginx:
> * On stop, use start-stop-daemon -R 1 to wait for the nginx processes
> to actually stop. This fixes a race condition with restart, where
> nginx fails to restart because start is called too early
> w.r.t. stop. (This only works with Debian's start-stop-daemon,
> however BusyBox's start-stop-daemon does not fail when given -R; it
> just ignores the argument silently).
> * Implement reload with an actual reload instead of a restart.
> * Add force-reload.
> Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
Committed to 2017.08.x, thanks.
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Buildroot] [PATCH 1/1] package/nginx/S50nginx: Do not assume start-stop-daemon knows -R.
2017-10-09 9:50 ` [Buildroot] [PATCH 1/1] package/nginx/S50nginx: Do not assume start-stop-daemon knows -R Thomas Claveirole
2017-10-10 15:54 ` Peter Korsgaard
@ 2017-10-17 9:07 ` Peter Korsgaard
1 sibling, 0 replies; 9+ messages in thread
From: Peter Korsgaard @ 2017-10-17 9:07 UTC (permalink / raw)
To: buildroot
>>>>> "Thomas" == Thomas Claveirole <thomas.claveirole@green-communications.fr> writes:
> start-stop-daemon fails on -R when not compiled with
> CONFIG_FEATURE_START_STOP_DAEMON_FANCY. Thus, do not rely on -R
> during stop to avoid a race condition during restart.
> Use a sleep 1 during restart instead, as suggested by Peter Korsgaard
> in <87bmluk4bm.fsf@dell.be.48ers.dk>.
> Signed-off-by: Thomas Claveirole <thomas.claveirole@green-communications.fr>
Committed to 2017.08.x, thanks.
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-17 9:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-25 13:20 [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Thomas Claveirole
2017-09-26 10:00 ` Johan Oudinet
2017-09-26 20:05 ` Samuel Martin
2017-09-26 22:39 ` Arnout Vandecappelle
2017-09-28 19:31 ` Peter Korsgaard
2017-10-09 9:50 ` [Buildroot] [PATCH 1/1] package/nginx/S50nginx: Do not assume start-stop-daemon knows -R Thomas Claveirole
2017-10-10 15:54 ` Peter Korsgaard
2017-10-17 9:07 ` Peter Korsgaard
2017-10-17 8:58 ` [Buildroot] [PATCH 1/1] nginx (S50nginx): Fix stop, reload, restart. Add force-reload Peter Korsgaard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.