Skip to content

Commit

Permalink
Merge pull request #172 from clue-labs/tcp-error
Browse files Browse the repository at this point in the history 
Improve error handling when sending TCP/IP data to DNS server fails (macOS)
  • Loading branch information
@WyriHaximus
WyriHaximus authored Feb 12, 2021
2 parents 5f8494c + 054d1d8 commit 134d424
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 11 deletions.
16 changes: 13 additions & 3 deletions src/Query/TcpTransportExecutor.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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();
Expand Down
104 changes: 97 additions & 7 deletions tests/Query/TcpTransportExecutorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,14 +245,53 @@ 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()
{
$writableCallback = null;
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');
}

$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');
Expand All @@ -262,10 +301,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);
}

Expand All @@ -283,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();
Expand Down
2 changes: 1 addition & 1 deletion tests/Query/UdpTransportExecutorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit 134d424

Please sign in to comment.