Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"name": "expensify/bedrock-php",
"description": "Bedrock PHP Library",
"type": "library",
"version": "2.2.2",
"version": "2.2.5",
"authors": [
{
"name": "Expensify",
Expand Down
40 changes: 38 additions & 2 deletions src/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -566,13 +566,49 @@ private function sendRawRequest(string $host, int $port, string $rawRequest)
throw new ConnectionFailure("Could not connect to create socket: $socketError");
}

// Configure this socket and try to connect to it
// PHP 8 Socket objects need explicit non-blocking mode to prevent EINPROGRESS timeouts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment confuses me very much (like this problem as a whole) 😅

  • Isn't our intention to use blocking sockets?
  • The next confused engineer will read "PHP 8 Socket objects need explicit non-blocking mode to prevent EINPROGRESS timeouts" and think: so why are we handling EINPROGRESS below?

socket_set_nonblock($this->socket);
socket_set_option($this->socket, SOL_SOCKET, SO_SNDTIMEO, ['sec' => $this->connectionTimeout, 'usec' => $this->connectionTimeoutMicroseconds]);
socket_set_option($this->socket, SOL_SOCKET, SO_RCVTIMEO, ['sec' => $this->readTimeout, 'usec' => $this->readTimeoutMicroseconds]);
$connectStart = microtime(true);
@socket_connect($this->socket, $host, $port);
$connectTime = (microtime(true) - $connectStart) * 1000; // Convert to milliseconds
$socketErrorCode = socket_last_error($this->socket);
if ($socketErrorCode === 115) {
$this->logger->info('Bedrock\Client - socket_connect returned error 115, continuing.');
$this->logger->info('Bedrock\Client - socket_connect returned error 115, waiting for connection to complete.', [
'host' => $host,
'connect_attempt_time_ms' => round($connectTime, 3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update to lowerCamelCase

'pid' => getmypid(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we logging pid here? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was originally looking to see if this could be tied to php-fpm workers shutting down after 1000 requests, but that did not pan out. I will remove it.

]);

// Wait for the socket to be ready for writing after EINPROGRESS
$write = [$this->socket];
$read = [];
$except = [];
$selectResult = socket_select($read, $write, $except, $this->connectionTimeout, $this->connectionTimeoutMicroseconds);

if ($selectResult === false) {
$socketError = socket_strerror(socket_last_error($this->socket));
throw new ConnectionFailure("socket_select failed after EINPROGRESS for $host:$port. Error: $socketError");
} elseif ($selectResult === 0) {
throw new ConnectionFailure("Socket not ready for writing within timeout after EINPROGRESS for $host:$port");
} elseif (empty($write)) {
$socketErrorCode = socket_last_error($this->socket);
$socketError = socket_strerror($socketErrorCode);
throw new ConnectionFailure("Socket had error after EINPROGRESS for $host:$port. Error: $socketErrorCode $socketError");
}

$selectTime = (microtime(true) - $connectStart) * 1000; // Total time from connect to ready
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect selectTime to time socket_select, not socket_select & socket_connect


// Set socket back to blocking mode for normal operations
socket_set_block($this->socket);

$this->logger->info('Bedrock\Client - Socket ready for writing after EINPROGRESS.', [
'host' => $host,
'total_connection_time_ms' => round($selectTime, 3),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here and below, lowerCameCase please

'select_wait_time_ms' => round($selectTime - $connectTime, 3),
'pid' => getmypid(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here

]);
} elseif ($socketErrorCode) {
$socketError = socket_strerror($socketErrorCode);
throw new ConnectionFailure("Could not connect to Bedrock host $host:$port. Error: $socketErrorCode $socketError");
Expand Down