Thread->start() blocks while another thread is trying to read stdin

[dktapps]

Environment

• PHP: 7.2.15
• pthreads: 8f4205d (this bug specifically occurs with this commit, reverting the commit removes the bug)
• OS: Windows 10 x64 1809

Reproducing Code

declare(strict_types=1);

class CommandReader extends \Thread{

	/** @var resource */
	private static $stdin;

	/** @var \Threaded */
	protected $buffer;

	public $reading = false;

	public function __construct(){
		$this->buffer = new \Threaded;
	}

	public function run(){
		self::$stdin = fopen("php://stdin", "r");
		$this->synchronized(function(){
			$this->reading = true;
			$this->notify();
		});

		fgets(self::$stdin);

		fclose(self::$stdin);
	}
}

$reader = new CommandReader();
$reader->start(PTHREADS_INHERIT_NONE);
$reader->synchronized(function() use($reader){
	while(!$reader->reading){
		$reader->wait();
	}
});

$w = new \Worker();
var_dump("before worker start");
$w->start();
var_dump("after worker start");

$w->shutdown();
var_dump("main thread die");

Expected Output

The script should hang after main thread die until the enter key is pressed.

Actual Output

The script hangs after before worker start until the enter key is pressed.

[dktapps]

Specifically this bug is caused by #831 . Reverting #831 fixes the problem.

[krakjoe]

Can you try this please

diff --git a/src/prepare.c b/src/prepare.c
index b9ed971..caa3e10 100644
--- a/src/prepare.c
+++ b/src/prepare.c
@@ -590,58 +590,83 @@ static inline void pthreads_prepare_ini(pthreads_object_t* thread) {
 } /* }}} */
 
 /* {{{ */
-static inline void pthreads_prepare_stdio_constants(pthreads_object_t* thread) {
-	zend_constant *zconstant;
-	zend_string *name;
-	int stdio_in, stdio_out, stdio_err;
-	
-	ZEND_HASH_FOREACH_STR_KEY_PTR(PTHREADS_EG(thread->creator.ls, zend_constants), name, zconstant) {
-		int fd = -1;
-		php_stream * stream = NULL;
-		const char *mode = NULL;
+typedef struct _php_stdio_stream_abstract_t {
+	FILE *fp;
+	int fd;
+} php_stdio_stream_abstract_t;
 
-		if (zconstant->name) {
-			stdio_in = strncmp(name->val, "STDIN", name->len-1)==0;
-			stdio_out = strncmp(name->val, "STDOUT", name->len-1)==0;
-			stdio_err = strncmp(name->val, "STDERR", name->len-1)==0;
+int pthreads_prepare_extract_fd(zval *zv) {
+	php_stream *stream = (php_stream*) Z_RES_P(zv)->ptr;
+	php_stdio_stream_abstract_t *stdio = 
+		(php_stdio_stream_abstract_t*) stream->abstract;
 
-			if((stdio_in || stdio_out || stdio_err) && !pthreads_constant_exists(name)) {
-				zend_constant constant;
+	fflush(stdio->fp);
 
-				if(stdio_in) {
-					fd = dup(STDIN_FILENO);
-					mode = "rb";
-				} else if(stdio_out) {
-					fd = dup(STDOUT_FILENO);
-					mode = "wb";
-				} else if(stdio_err) {
-					fd = dup(STDERR_FILENO);
-					mode = "wb";
-				}
+	return dup(stdio->fd);
+} /* }}} */
 
-				if (fd == -1)	{
-					continue;
-				}
-				stream = php_stream_fopen_from_fd(fd, mode, NULL);
+/* {{{ */
+static inline void pthreads_prepare_stdio_constants(pthreads_object_t* thread) {
+	char *names[] = {
+		"STDIN",
+		"STDOUT",
+		"STDERR",
+		NULL
+	};
+	char *modes[] = {
+		"rb",
+		"wb",
+		"wb",
+		NULL
+	};
+
+	char **name = names;
+	char **mode = modes;
+
+	while (*name) {
+		zend_constant *oconstant = zend_hash_str_find_ptr(
+			PTHREADS_EG(thread->creator.ls, zend_constants), *name, strlen(*name)),
+			       nconstant;
+		php_stream *stream;
+
+		if (Z_TYPE(oconstant->value) != IS_RESOURCE) {
+			name++;
+			mode++;
+			continue;
+		}
+		
+		stream = php_stream_fopen_from_fd(
+			pthreads_prepare_extract_fd(&oconstant->value), *mode, NULL);
 
-				if (stream == NULL) {
-					close(fd);
-					continue;
-				}
-				php_stream_to_zval(stream,  &constant.value);
+		if (!stream) {
+			name++;
+			mode++;
+			continue;
+		}
+
+		php_stream_to_zval(
+			stream,  &nconstant.value);
 
+		if (Z_TYPE(nconstant.value) != IS_RESOURCE) {
+			name++;
+			mode++;
+			continue;
+		}
+
+		
 #if PHP_VERSION_ID < 70300
-				constant.flags = zconstant->flags;
-				constant.module_number = zconstant->module_number;
+		nconstant.flags = oconstant->flags;
+		nconstant.module_number = oconstant->module_number;
 #else
-				ZEND_CONSTANT_SET_FLAGS(&constant, ZEND_CONSTANT_FLAGS(zconstant), ZEND_CONSTANT_MODULE_NUMBER(zconstant));
+		ZEND_CONSTANT_SET_FLAGS(&nconstant, ZEND_CONSTANT_FLAGS(oconstant), ZEND_CONSTANT_MODULE_NUMBER(oconstant));
 #endif
-				constant.name = zend_string_new(name);
+		nconstant.name = zend_string_init(*name, strlen(*name), 0);
 
-				zend_register_constant(&constant);
-			}
-		}
-	} ZEND_HASH_FOREACH_END();
+		zend_register_constant(&nconstant);
+
+		name++;
+		mode++;
+	}
 } /* }}} */
 
 /* {{{ */

On a side note, that logic was really over complicated, looping over the whole constant table to work with three constants makes no sense.

I have a feeling this is more to do with flushing than anything else, hence the patch ... it may not be ...

I think you guys need to decide what behaviour you want from stdio streams, I'm not precisely sure that dup'ing is the right thing to do, and it isn't necessary either. That patch replicates the behaviour you seem to have chosen whatever ...

[joeyhub]

Wouldn't it be easier to just put that on a branch?