nginx (S50nginx): Fix stop, reload, restart. Add force-reload.

PATCH New Thomas Claveirole 5 participants 9 messages Sep 25, 2017 – Oct 17, 2017

lore

Share & tools

lore

Thomas Claveirole

author

2017-09-25 13:20 UTC #0 lore raw

PATCH 1/1 nginx (S50nginx): Fix stop, reload, restart. Add force-reload.

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 <[email protected]>

---
 package/nginx/S50nginx | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

package/nginx/S50nginx +13 -5

index b2f8b80855..a854c651d1 100755
@@ -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

Signature · 1 line

2.14.1

Johan Oudinet

2017-09-26 10:00 UTC #1 lore raw

PATCH 1/1 nginx (S50nginx): Fix stop, reload, restart. Add force-reload.

Hello,

Quoted 60 lines · Thomas Claveirole

Fix package/nginx/S50nginx:

* Implement reload with an actual reload instead of a restart.

* Add force-reload.

Signed-off-by Thomas Claveirole <[email protected]>

---
 package/nginx/S50nginx | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

package/nginx/S50nginx +13 -6

index b2f8b80855..a854c651d1 100755
@@ -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
--

Quoted 1 line

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,

Signature · 1 line

Johan

Samuel Martin

2017-09-26 20:05 UTC #2 lore raw

PATCH 1/1 nginx (S50nginx): Fix stop, reload, restart. Add force-reload.

In reply to Thomas Claveirole

Quoted 9 lines · Thomas Claveirole

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

Quoted 5 lines

* Implement reload with an actual reload instead of a restart.
* Add force-reload.
Signed-off-by Thomas Claveirole <[email protected]>

Reviewed-by Samuel Martin <[email protected]>

Regards,

Signature · 1 line

Samuel

Arnout Vandecappelle

Applied

2017-09-26 22:39 UTC #3 lore raw

PATCH 1/1 nginx (S50nginx): Fix stop, reload, restart. Add force-reload.

In reply to Thomas Claveirole

Quoted 9 lines · Thomas Claveirole

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

Quoted 5 lines

* Implement reload with an actual reload instead of a restart.
* Add force-reload.
Signed-off-by Thomas Claveirole <[email protected]>

Applied to master, thanks.

Regards,
Arnout

Quoted 46 lines

---
 package/nginx/S50nginx | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

package/nginx/S50nginx +13 -5

index b2f8b80855..a854c651d1 100755
@@ -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

Signature · 6 lines

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

Peter Korsgaard

2017-09-28 19:31 UTC #4 lore raw

PATCH 1/1 nginx (S50nginx): Fix stop, reload, restart. Add force-reload.

In reply to Thomas Claveirole

Quoted 35 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Thomas" == Thomas Claveirole <[email protected]> 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 <[email protected]>
---
 package/nginx/S50nginx | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
package/nginx/S50nginx +1 -1
index b2f8b80855..a854c651d1 100755
@@ -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

package/nginx/S50nginx line 20

Quoted 10 lines

package/nginx/S50nginx +6 -2

+	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?

Quoted 22 lines

package/nginx/S50nginx +6 -2

 	;;
-  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

Signature · 6 lines

2.14.1

_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Signature · 1 line

Bye, Peter Korsgaard

Thomas Claveirole

author

2017-10-09 09:50 UTC #5 lore raw

PATCH 1/1 package/nginx/S50nginx: Do not assume start-stop-daemon knows -R.

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 <[email protected]>.

Signed-off-by Thomas Claveirole <[email protected]>

---
 package/nginx/S50nginx | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

package/nginx/S50nginx +2 -4

index a854c651d1..964652b922 100755
@@ -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
 	;;
   *)

Signature · 1 line

2.14.2

Peter Korsgaard

2017-10-10 15:54 UTC #6 lore raw

PATCH 1/1 package/nginx/S50nginx: Do not assume start-stop-daemon knows -R.

Quoted 10 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Thomas" == Thomas Claveirole <[email protected]> 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 <[email protected]>.
Signed-off-by Thomas Claveirole <[email protected]>

Committed, thanks.

Signature · 1 line

Bye, Peter Korsgaard

Peter Korsgaard

2017-10-17 09:07 UTC #7 lore raw

PATCH 1/1 package/nginx/S50nginx: Do not assume start-stop-daemon knows -R.

In reply to Thomas Claveirole

Quoted 10 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Thomas" == Thomas Claveirole <[email protected]> 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 <[email protected]>.
Signed-off-by Thomas Claveirole <[email protected]>

Committed to 2017.08.x, thanks.

Signature · 1 line

Bye, Peter Korsgaard

Peter Korsgaard

2017-10-17 08:58 UTC #8 lore raw

PATCH 1/1 nginx (S50nginx): Fix stop, reload, restart. Add force-reload.

In reply to Thomas Claveirole

Quoted 10 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Thomas" == Thomas Claveirole <[email protected]> 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).

Quoted 5 lines

* Implement reload with an actual reload instead of a restart.
* Add force-reload.
Signed-off-by Thomas Claveirole <[email protected]>

Committed to 2017.08.x, thanks.

Signature · 1 line

Bye, Peter Korsgaard