From 43c1e8d3bf90708e7d3b55b5cd514b6d358a0335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Thu, 11 Feb 2021 08:14:48 +0100 Subject: [PATCH 1/2] Skip flaky test on macOS due to possible kernel race condition --- tests/Query/TcpTransportExecutorTest.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/Query/TcpTransportExecutorTest.php b/tests/Query/TcpTransportExecutorTest.php index 21583077..7671ed45 100644 --- a/tests/Query/TcpTransportExecutorTest.php +++ b/tests/Query/TcpTransportExecutorTest.php @@ -247,6 +247,14 @@ function ($e) use (&$wait) { public function testQueryStaysPendingWhenClientCanNotSendExcessiveMessageInOneChunkWhenServerClosesSocket() { + if (PHP_OS === 'Darwin') { + // Skip on macOS because it exhibits what looks like a kernal race condition when sending excessive data to a socket that is about to shut down (EPROTOTYPE) + // Due to this race condition, this is somewhat flaky. Happens around 75% of the time, use `--repeat=100` to reproduce. + // fwrite(): Send of 4260000 bytes failed with errno=41 Protocol wrong type for socket + // @link http://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/ + $this->markTestSkipped('Skipped on macOS due to possible race condition'); + } + $writableCallback = null; $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); $loop->expects($this->once())->method('addWriteStream')->with($this->anything(), $this->callback(function ($cb) use (&$writableCallback) { @@ -262,10 +270,10 @@ public function testQueryStaysPendingWhenClientCanNotSendExcessiveMessageInOneCh $address = stream_socket_get_name($server, false); $executor = new TcpTransportExecutor($address, $loop); - $query = new Query('google' . str_repeat('.com', 10000), Message::TYPE_A, Message::CLASS_IN); + $query = new Query('google' . str_repeat('.com', 100), Message::TYPE_A, Message::CLASS_IN); // send a bunch of queries and keep reference to last promise - for ($i = 0; $i < 100; ++$i) { + for ($i = 0; $i < 2000; ++$i) { $promise = $executor->query($query); } From 054d1d8462ffe1659cb17da6d2fc8f432bd715d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 12 Feb 2021 13:54:20 +0100 Subject: [PATCH 2/2] Improve error handling when sending TCP/IP data to DNS server fails --- src/Query/TcpTransportExecutor.php | 16 ++++- tests/Query/TcpTransportExecutorTest.php | 92 ++++++++++++++++++++++-- tests/Query/UdpTransportExecutorTest.php | 2 +- 3 files changed, 101 insertions(+), 9 deletions(-) diff --git a/src/Query/TcpTransportExecutor.php b/src/Query/TcpTransportExecutor.php index 4ec232e1..05dab587 100644 --- a/src/Query/TcpTransportExecutor.php +++ b/src/Query/TcpTransportExecutor.php @@ -184,6 +184,9 @@ public function query(Query $query) // set socket to non-blocking and wait for it to become writable (connection success/rejected) \stream_set_blocking($socket, false); + if (\function_exists('stream_set_chunk_size')) { + \stream_set_chunk_size($socket, (1 << 31) - 1); // @codeCoverageIgnore + } $this->socket = $socket; } @@ -234,7 +237,12 @@ public function handleWritable() $written = @\fwrite($this->socket, $this->writeBuffer); if ($written === false || $written === 0) { - $this->closeError('Unable to write to closed socket'); + $error = \error_get_last(); + \preg_match('/errno=(\d+) (.+)/', $error['message'], $m); + $this->closeError( + 'Unable to send query to DNS server (' . (isset($m[2]) ? $m[2] : $error['message']) . ')', + isset($m[1]) ? (int) $m[1] : 0 + ); return; } @@ -300,8 +308,9 @@ public function handleRead() /** * @internal * @param string $reason + * @param int $code */ - public function closeError($reason) + public function closeError($reason, $code = 0) { $this->readBuffer = ''; if ($this->readPending) { @@ -325,7 +334,8 @@ public function closeError($reason) foreach ($this->names as $id => $name) { $this->pending[$id]->reject(new \RuntimeException( - 'DNS query for ' . $name . ' failed: ' . $reason + 'DNS query for ' . $name . ' failed: ' . $reason, + $code )); } $this->pending = $this->names = array(); diff --git a/tests/Query/TcpTransportExecutorTest.php b/tests/Query/TcpTransportExecutorTest.php index 7671ed45..0d6d988f 100644 --- a/tests/Query/TcpTransportExecutorTest.php +++ b/tests/Query/TcpTransportExecutorTest.php @@ -245,6 +245,41 @@ function ($e) use (&$wait) { $this->assertFalse($wait); } + public function testQueryStaysPendingWhenClientCanNotSendExcessiveMessageInOneChunk() + { + $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + $loop->expects($this->once())->method('addWriteStream'); + $loop->expects($this->once())->method('addReadStream'); + $loop->expects($this->never())->method('removeWriteStream'); + $loop->expects($this->never())->method('removeReadStream'); + + $server = stream_socket_server('tcp://127.0.0.1:0'); + + $address = stream_socket_get_name($server, false); + $executor = new TcpTransportExecutor($address, $loop); + + $query = new Query('google' . str_repeat('.com', 100), Message::TYPE_A, Message::CLASS_IN); + + // send a bunch of queries and keep reference to last promise + for ($i = 0; $i < 8000; ++$i) { + $promise = $executor->query($query); + } + + $client = stream_socket_accept($server); + assert(is_resource($client)); + + $executor->handleWritable(); + + $promise->then(null, 'printf'); + $promise->then($this->expectCallableNever(), $this->expectCallableNever()); + + $ref = new \ReflectionProperty($executor, 'writePending'); + $ref->setAccessible(true); + $writePending = $ref->getValue($executor); + + $this->assertTrue($writePending); + } + public function testQueryStaysPendingWhenClientCanNotSendExcessiveMessageInOneChunkWhenServerClosesSocket() { if (PHP_OS === 'Darwin') { @@ -255,12 +290,8 @@ public function testQueryStaysPendingWhenClientCanNotSendExcessiveMessageInOneCh $this->markTestSkipped('Skipped on macOS due to possible race condition'); } - $writableCallback = null; $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); - $loop->expects($this->once())->method('addWriteStream')->with($this->anything(), $this->callback(function ($cb) use (&$writableCallback) { - $writableCallback = $cb; - return true; - })); + $loop->expects($this->once())->method('addWriteStream'); $loop->expects($this->once())->method('addReadStream'); $loop->expects($this->never())->method('removeWriteStream'); $loop->expects($this->never())->method('removeReadStream'); @@ -291,6 +322,57 @@ public function testQueryStaysPendingWhenClientCanNotSendExcessiveMessageInOneCh $this->assertTrue($writePending); } + public function testQueryRejectsWhenClientKeepsSendingWhenServerClosesSocket() + { + $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + $loop->expects($this->once())->method('addWriteStream'); + $loop->expects($this->once())->method('addReadStream'); + $loop->expects($this->once())->method('removeWriteStream'); + $loop->expects($this->once())->method('removeReadStream'); + + $server = stream_socket_server('tcp://127.0.0.1:0'); + + $address = stream_socket_get_name($server, false); + $executor = new TcpTransportExecutor($address, $loop); + + $query = new Query('google' . str_repeat('.com', 100), Message::TYPE_A, Message::CLASS_IN); + + // send a bunch of queries and keep reference to last promise + for ($i = 0; $i < 2000; ++$i) { + $promise = $executor->query($query); + } + + $client = stream_socket_accept($server); + fclose($client); + + $executor->handleWritable(); + + $ref = new \ReflectionProperty($executor, 'writePending'); + $ref->setAccessible(true); + $writePending = $ref->getValue($executor); + + // We expect an EPIPE (Broken pipe) on second write. + // However, macOS may report EPROTOTYPE (Protocol wrong type for socket) on first write due to kernel race condition. + // fwrite(): Send of 4260000 bytes failed with errno=41 Protocol wrong type for socket + // @link http://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/ + if ($writePending) { + $executor->handleWritable(); + } + + $exception = null; + $promise->then(null, function ($reason) use (&$exception) { + $exception = $reason; + }); + + // expect EPIPE (Broken pipe), except for macOS kernel race condition or legacy HHVM + $this->setExpectedException( + 'RuntimeException', + 'Unable to send query to DNS server', + defined('SOCKET_EPIPE') && !defined('HHVM_VERSION') ? (PHP_OS !== 'Darwin' || $writePending ? SOCKET_EPIPE : SOCKET_EPROTOTYPE) : null + ); + throw $exception; + } + public function testQueryRejectsWhenServerClosesConnection() { $loop = Factory::create(); diff --git a/tests/Query/UdpTransportExecutorTest.php b/tests/Query/UdpTransportExecutorTest.php index 5a202ace..25ce4429 100644 --- a/tests/Query/UdpTransportExecutorTest.php +++ b/tests/Query/UdpTransportExecutorTest.php @@ -153,7 +153,7 @@ public function testQueryRejectsIfSendToServerFailsAfterConnection() $exception = $reason; }); - // ECONNREFUSED( Connection refused) on Linux, EMSGSIZE (Message too long) on macOS + // ECONNREFUSED (Connection refused) on Linux, EMSGSIZE (Message too long) on macOS $this->setExpectedException('RuntimeException', 'Unable to send query to DNS server'); throw $exception; }