-
Notifications
You must be signed in to change notification settings - Fork 8k
Add stream socket keepalive context options #20381
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
Conversation
|
Is it possible please to also have TCP_USER_TIMEOUT constant added to sockets and made available to be set in the context along with this change? Helps in detecting half closed connections and goes hand in hand with KEEPALIVE https://blog.cloudflare.com/when-tcp-sockets-refuse-to-die/ This is how I currently need to do it, being able to provide it via context would be a big benefit (For example I need to hack around in ReactPHP/Socket to be able to set this flag because the stream is not easily accessible but context can be provided) if (!defined('TCP_USER_TIMEOUT')) {
define("TCP_USER_TIMEOUT", 18);
}
$internalSocket = socket_import_stream($stream);
socket_setopt($internalSocket, SOL_TCP, TCP_USER_TIMEOUT, 15000); //ms |
|
Yeah that could be added but probs in different PR. |
main/streams/xp_socket.c
Outdated
| #else | ||
| #define PHP_STREAM_XPORT_IS_UNIX(_stream) false | ||
| #endif | ||
| #define PHP_STREAM_XPORT_IS_UDP(_stream) (_stream->ops == &php_stream_udp_socket_ops) |
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 should replace those with php_stream_is
51e70d5 to
5dc974b
Compare
This adds so_keepalive, tcp_keepidle, tcp_keepintvl and tcp_keepcnt stream socket context options that are used to set their upper case C macro variants by setsockopt function. The test requires sockets extension and just tests that the values are being set. This is because a real test would be slow and difficult to show that those options really work due to how they work internally. Closes phpGH-20381
|
@lucasnetau I have been looking to TCP_USER_TIMEOUT and I can see how it can be useful. However, as far as I see, there are no similar options in other platforms (Windows, FreeBSD, MacOS) so I don't think we should directly expose it in streams. I would say that using socket_import_stream might be better here as one can check if the TCP_USER_TIMEOUT is available and then have the right expectation. Generally I think that the platform specific options should go only to socket extension. |
This adds so_keepalive, tcp_keepidle, tcp_keepintvl and tcp_keepcnt stream socket context options that are used to set their upper case C macro variants by setsockopt function. The test requires sockets extension and just tests that the values are being set. This is because a real test would be slow and difficult to show that those options really work due to how they work internally. Closes phpGH-20381
5dc974b to
0fd8aae
Compare
This adds so_keepalive, tcp_keepidle, tcp_keepintvl and tcp_keepcnt stream socket context options that are used to set their upper case C macro variants by setsockopt function.
The test requires sockets extension and just tests that the values are being set. This is because a real test would be slow and difficult to show that those options really work due to how they work internally.