-
Notifications
You must be signed in to change notification settings - Fork 12
Gracefully handle EINPROGRESS #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
| 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I get this, wouldn't
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
|
|
||




There was a problem hiding this comment.
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_connectand then again here.There was a problem hiding this comment.
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