Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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.1.2",
"version": "2.1.3",
"authors": [
{
"name": "Expensify",
Expand Down
24 changes: 23 additions & 1 deletion src/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,29 @@ private function sendRawRequest(string $host, int $port, string $rawRequest)
@socket_connect($this->socket, $host, $port);
$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 EINPROGRESS, waiting for connection to complete');

// Use select to wait for the socket to become writable (indicates connection completion)
$write = [$this->socket];
$read = null;
$except = null;

$selectResult = socket_select($read, $write, $except, $this->connectionTimeout, $this->connectionTimeoutMicroseconds);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it matters much, but this would basically double the timeout (since we would wait for the timeout in socket_connect and then again 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 think that's fine if we lower the timeout to 500ms, which I'll do

if ($selectResult === false) {
$socketError = socket_strerror(socket_last_error());
throw new ConnectionFailure("Failed to select on socket for host $host:$port. Error: $socketError");
} elseif ($selectResult === 0) {
throw new ConnectionFailure("Connection timeout while waiting for EINPROGRESS completion for host $host:$port");
}

// Check if connection completed successfully
$connectionError = socket_get_option($this->socket, SOL_SOCKET, SO_ERROR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I get this, wouldn't socket_select already return an error and that should be handled above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image image image image

This all sounds right to me 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

ok not sure really, but we can try it

if ($connectionError !== 0) {
$socketError = socket_strerror($connectionError);
throw new ConnectionFailure("Connection failed after EINPROGRESS for host $host:$port. Error: $connectionError $socketError");
}

$this->logger->info('Bedrock\Client - EINPROGRESS connection completed successfully');
} elseif ($socketErrorCode) {
$socketError = socket_strerror($socketErrorCode);
throw new ConnectionFailure("Could not connect to Bedrock host $host:$port. Error: $socketErrorCode $socketError");
Expand Down