Skip to content

Conversation

@flodnv
Copy link
Contributor

@flodnv flodnv commented Jul 31, 2025

image

@flodnv flodnv self-assigned this Jul 31, 2025
}

// 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

$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

@flodnv flodnv marked this pull request as ready for review August 1, 2025 07:59
iwiznia
iwiznia previously approved these changes Aug 4, 2025
@flodnv
Copy link
Contributor Author

flodnv commented Aug 4, 2025

Version created

@flodnv flodnv merged commit 4b813ec into main Aug 4, 2025
2 checks passed
@flodnv flodnv deleted the flo_EINPROGRESS branch August 4, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants