Skip to content
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

Better socket_timeout handling #977

Merged
merged 2 commits into from
Dec 16, 2023

Conversation

mlarraz
Copy link
Contributor

@mlarraz mlarraz commented Dec 9, 2023

  • Use Socket's connect_timeout (available since Ruby 3.0) instead of Timeout when possible

  • Pass socket_timeout down to the socket's send and recieve timeouts.

* Use Socket's connect_timeout (available since Ruby 3.0) instead of Timeout
  when possible

* Pass socket_timeout down to the socket's send and recieve timeouts.
@mlarraz
Copy link
Contributor Author

mlarraz commented Dec 14, 2023

@petergoldstein This is passing locally, do you have any feedback?

@petergoldstein
Copy link
Owner

@mlarraz Looks good other than the Rubocop issues. Can you please address those? Thanks.

@petergoldstein
Copy link
Owner

Thanks @mlarraz . This looks good. Merging.

@petergoldstein petergoldstein merged commit 6165b94 into petergoldstein:main Dec 16, 2023
19 checks passed
@mlarraz mlarraz deleted the socket_timeout branch December 17, 2023 00:00
mpg-tazuko-fukuda

This comment was marked as off-topic.

Comment on lines +119 to +124
seconds, fractional = options[:socket_timeout].divmod(1)
microseconds = fractional * 1_000_000
timeval = [seconds, microseconds].pack('l_2')

sock.setsockopt(::Socket::SOL_SOCKET, ::Socket::SO_RCVTIMEO, timeval)
sock.setsockopt(::Socket::SOL_SOCKET, ::Socket::SO_SNDTIMEO, timeval)
Copy link

Choose a reason for hiding this comment

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

I wonder if SO_RCVTIMEO/SO_SNDTIMEO have any effect on CRuby since 3.0 because sockets are nonblocking by default, unless Dalli sets them as blocking perharps?
man 7 sockets says:

SO_RCVTIMEO and SO_SNDTIMEO
[...] Timeouts only have effect for system calls that perform
socket I/O (e.g., read(2), recvmsg(2), send(2), sendmsg(2)); timeouts have no effect for select(2), poll(2),
epoll_wait(2), and so on.

Also the [seconds, microseconds].pack('l_2') feels dangerous because the definition of struct timeval is not that trivial, for instance on my machine:

struct timeval {
	__kernel_old_time_t	tv_sec;		/* seconds */
	__kernel_suseconds_t	tv_usec;	/* microseconds */
};

and

typedef __kernel_long_t		__kernel_suseconds_t;
typedef long		__kernel_long_t;

So the microseconds seems to be long so 64-bit, but pack's l is 32-bit, so that seems incorrect.

Copy link

Choose a reason for hiding this comment

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

I wanted to make sure, so:

#include <stdio.h>
#include <sys/socket.h>

int main(int argc, char const *argv[]) {
  printf("sizeof(struct timeval) = %d\n", sizeof(struct timeval));
}

it gives:

sizeof(struct timeval) = 16

But I made a mistake above, pack's l_ is signed long, native endian (confusing, given l is 32-bit signed, native endian (int32_t)).
So .pack('l_2') does produce 16 bytes.
I think it's still very brittle and might be incorrect on some platforms though (maybe on 32-bit platforms, or on 64-bit platforms where tv_usec is represented as a 32-bit field and then depending on endian it breaks or not).

Copy link

Choose a reason for hiding this comment

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

Consider using IO#timeout.

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.

5 participants