Simplify the ZTS reader count, wait for restarts of other processes

- Enter and leave share one function, accel_zts_readers_add(): the
  lock-free path changes a positive count that stays positive, the
  0 <-> 1 transitions take the mutex and call fcntl().
- A thread that becomes the first reader of its process while another
  process restarts the cache now waits for the restart (F_SETLKW)
  instead of running its whole request uncached and resetting its PCRE
  cache.
- Comments on opcache.force_restart_timeout under ZTS: the restart
  waits for running requests of the own process, other processes are
  killed with all their threads, as without ZTS.
- Test: opcache_reset() in the CLI server with two workers; the restart
  must happen in the next request and the cache must work after it. A
  reference that is never released makes it fail.

Author: pavimus

ext/opcache/ZendAccelerator.c

@@ -344,7 +344,7 @@ static MUTEX_T accel_zts_readers_mutex = NULL;
 static zend_atomic_int accel_zts_readers;
 static time_t accel_zts_next_warning;
 
-static bool accel_zts_lock_reader_byte(int type)
+static bool accel_zts_lock_reader_byte(int cmd, int type)
 {
 	struct flock lock;
 
@@ -353,59 +353,39 @@ static bool accel_zts_lock_reader_byte(int type)
 	lock.l_start = 1;
 	lock.l_len = 1;
 
-	return fcntl(lock_file, F_SETLK, &lock) != -1;
+	return fcntl(lock_file, cmd, &lock) != -1;
 }
 
-/* Must only be called from RINIT; on success the caller sets ZCG(counted) */
-static bool accel_zts_reader_enter(void)
+/* Adds (+1, only from RINIT) or releases (-1) a reference of the calling thread.
+ * Does not use ZCG() as it is also called from accel_globals_dtor(). */
+static bool accel_zts_readers_add(int delta)
 {
 	int count;
 
 #ifndef HAVE_NO_ATOMICS
-	/* the process already holds the reader lock: only a positive count may grow */
 	count = zend_atomic_int_load_ex(&accel_zts_readers);
-	while (count > 0) {
-		if (zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count + 1)) {
+	while (count > 0 && count + delta > 0) {
+		if (zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count + delta)) {
 			return true;
 		}
 	}
 #endif
 
 	tsrm_mutex_lock(accel_zts_readers_mutex);
 	count = zend_atomic_int_load_ex(&accel_zts_readers);
-	/* fails while another process restarts the cache (it holds the write lock) */
-	if (count == 0 && !accel_zts_lock_reader_byte(F_RDLCK)) {
+	/* waits while another process restarts the cache (it holds the write lock) */
+	if (count == 0 && !accel_zts_lock_reader_byte(F_SETLKW, F_RDLCK)) {
 		zend_accel_error(ACCEL_LOG_DEBUG, "UpdateC(+1):  %s (%d)", strerror(errno), errno);
 		tsrm_mutex_unlock(accel_zts_readers_mutex);
 		return false;
 	}
-	while (!zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count + 1));
-	tsrm_mutex_unlock(accel_zts_readers_mutex);
-	return true;
-}
-
-/* Releases one reference; does not use ZCG() as it is also called from accel_globals_dtor() */
-static void accel_zts_reader_leave(void)
-{
-	int count;
-
-#ifndef HAVE_NO_ATOMICS
-	count = zend_atomic_int_load_ex(&accel_zts_readers);
-	while (count > 1) {
-		if (zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count - 1)) {
-			return;
-		}
-	}
-#endif
-
-	tsrm_mutex_lock(accel_zts_readers_mutex);
-	count = zend_atomic_int_load_ex(&accel_zts_readers);
-	while (!zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count - 1));
-	if (count == 1) {
+	while (!zend_atomic_int_compare_exchange_ex(&accel_zts_readers, &count, count + delta));
+	if (count + delta == 0) {
 		/* the last reader of this process; unlock before another thread may relock */
-		accel_zts_lock_reader_byte(F_UNLCK);
+		accel_zts_lock_reader_byte(F_SETLK, F_UNLCK);
 	}
 	tsrm_mutex_unlock(accel_zts_readers_mutex);
+	return true;
 }
 #endif
 
@@ -417,7 +397,7 @@ static inline zend_result accel_activate_add(void)
 	INCREMENT(mem_usage);
 	SHM_PROTECT();
 #elif defined(ZTS)
-	/* A thread becomes a reader only in RINIT, see accel_zts_reader_enter(). A request
+	/* A thread becomes a reader only in RINIT, see accel_zts_readers_add(). A request
 	 * that was not admitted there must not start to use SHM later. */
 	return FAILURE;
 #else
@@ -471,7 +451,7 @@ static inline void accel_unlock_all(void)
 	 * belong to the process, and the SHM write lock (byte 0) and the restart lock
 	 * (byte 2) may be held by other threads. */
 	if (ZCG(counted)) {
-		accel_zts_reader_leave();
+		accel_zts_readers_add(-1);
 	}
 #else
 	if (lock_file == -1) {
@@ -1032,32 +1012,34 @@ static inline bool accel_is_inactive(void)
 static bool accel_zts_restart_begin(void)
 {
 	int readers;
+	time_t now;
 
 	tsrm_mutex_lock(accel_zts_readers_mutex);
 	readers = zend_atomic_int_load_ex(&accel_zts_readers);
-	if (readers == 0 && accel_zts_lock_reader_byte(F_WRLCK)) {
+	if (readers == 0 && accel_zts_lock_reader_byte(F_SETLK, F_WRLCK)) {
 		return true;
 	}
 	tsrm_mutex_unlock(accel_zts_readers_mutex);
 
+	now = time(NULL);
 	if (readers == 0) {
-		/* readers in other processes: keep the force_restart_timeout handling */
+		/* readers in other processes: as without ZTS, force_restart_timeout kills them,
+		 * here with all their threads */
 		(void) accel_is_inactive();
-	} else if (ZCG(accel_directives).force_restart_timeout
-			&& ZCSG(force_restart_time)
-			&& time(NULL) >= ZCSG(force_restart_time)
-			&& time(NULL) >= accel_zts_next_warning) {
-		/* threads cannot be killed, and this process must not kill itself */
+	} else if (ZCG(accel_directives).force_restart_timeout && ZCSG(force_restart_time)
+			&& now >= ZCSG(force_restart_time) && now >= accel_zts_next_warning) {
+		/* Threads cannot be killed and this process must not kill itself: the restart
+		 * waits for the running requests, new ones run uncached meanwhile */
 		zend_accel_error(ACCEL_LOG_WARNING, "Restart delayed for %ld seconds by %d threads of process %d still using the cache",
-			(long)(time(NULL) - ZCSG(force_restart_time) + ZCG(accel_directives).force_restart_timeout), readers, getpid());
-		accel_zts_next_warning = time(NULL) + ZCG(accel_directives).force_restart_timeout;
+			(long)(now - ZCSG(force_restart_time) + ZCG(accel_directives).force_restart_timeout), readers, getpid());
+		accel_zts_next_warning = now + ZCG(accel_directives).force_restart_timeout;
 	}
 	return false;
 }
 
 static void accel_zts_restart_end(void)
 {
-	accel_zts_lock_reader_byte(F_UNLCK);
+	accel_zts_lock_reader_byte(F_SETLK, F_UNLCK);
 	accel_zts_next_warning = 0;
 	tsrm_mutex_unlock(accel_zts_readers_mutex);
 }
@@ -2886,7 +2868,7 @@ ZEND_RINIT_FUNCTION(zend_accelerator)
 #if defined(ZTS) && !defined(ZEND_WIN32)
 	/* Under ZTS this is the only place where a request starts to use SHM */
 	if (ZCG(accelerator_enabled)) {
-		if (accel_zts_reader_enter()) {
+		if (accel_zts_readers_add(1)) {
 			ZCG(counted) = true;
 			/* SHM-backed keys may be added to the PCRE cache again */
 			ZCG(pcre_reseted) = false;
@@ -3127,7 +3109,7 @@ static void accel_globals_dtor(zend_accel_globals *accel_globals)
 	 * all threads from the current one. */
 	if (accel_globals->counted && accel_zts_readers_mutex) {
 		accel_globals->counted = false;
-		accel_zts_reader_leave();
+		accel_zts_readers_add(-1);
 	}
 #endif
 	zend_string_free(accel_globals->key);

ext/opcache/tests/gh8739.phpt

@@ -0,0 +1,55 @@
+--TEST--
+GH-8739: OPcache restart in a server with several workers
+--DESCRIPTION--
+A scheduled restart happens in the next request once no request uses the cache, and the cache
+works after it. Under ZTS this checks the per-process reader count and the reader lock shared by
+the worker processes: a reference that is never released would delay the restart forever.
+--EXTENSIONS--
+opcache
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+--CONFLICTS--
+server
+--FILE--
+<?php
+$dir = __DIR__ . '/gh8739';
+@mkdir($dir);
+file_put_contents("$dir/status.php", <<<'PHP'
+<?php
+$status = opcache_get_status(false);
+echo $status['opcache_enabled'] ? 'enabled' : 'disabled',
+    ', restarts: ', $status['opcache_statistics']['manual_restarts'],
+    ', ', opcache_is_script_cached(__FILE__) ? 'cached' : 'not cached', "\n";
+PHP);
+file_put_contents("$dir/reset.php", "<?php\nvar_dump(opcache_reset());\n");
+
+include __DIR__ . '/php_cli_server.inc';
+
+if (PHP_OS_FAMILY !== 'Windows') {
+    putenv('PHP_CLI_SERVER_WORKERS=2');
+}
+$ini = '-d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.file_update_protection=0';
+$ext = ini_get('extension_dir');
+if (file_exists($ext . DIRECTORY_SEPARATOR . (PHP_OS_FAMILY === 'Windows' ? 'php_opcache.dll' : 'opcache.so'))) {
+    $ini .= ' -d extension_dir=' . $ext . ' -d zend_extension=opcache';
+}
+php_cli_server_start($ini);
+
+$base = 'http://' . PHP_CLI_SERVER_ADDRESS . '/gh8739/';
+foreach (['status', 'reset', 'status', 'status'] as $script) {
+    echo @file_get_contents($base . $script . '.php') ?: "no response\n";
+}
+?>
+--CLEAN--
+<?php
+$dir = __DIR__ . '/gh8739';
+@unlink("$dir/status.php");
+@unlink("$dir/reset.php");
+@rmdir($dir);
+?>
+--EXPECT--
+enabled, restarts: 0, cached
+bool(true)
+enabled, restarts: 1, cached
+enabled, restarts: 1, cached