Project

General

Profile

Actions

Feature #5101

closed

allow optional timeout for TCPSocket.new

Added by normalperson (Eric Wong) over 12 years ago. Updated over 12 years ago.

Status:
Closed
Target version:
[ruby-core:38538]

Description

Proposed API would be:

TCPSocket.new(remote_host, remote_port,
connect_timeout: 0.5, local_host: nil, local_port: nil)

Or:

TCPSocket.new(remote_host, remote_port, connect_timeout: 0.5)

For the common case.

This would only timeout for establishing the TCP connection, not DNS
resolution. DNS resolution can be covered by Feature #5100.

The existing form for the (rarely-used) local_host/local_port args will
still be supported for backwards compatibility:

TCPSocket.new(remote_host, remote_port, local_host=nil, local_port=nil)

The current construct for doing a non-blocking connect with timeout
is too verbose:

addr = Socket.pack_sockaddr_in(9418, "127.0.0.1")
s = Socket.new(:AF_INET, :SOCK_STREAM, 0)
begin
  s.connect_nonblock(addr)
rescue Errno::EINPROGRESS
  IO.select(nil, [s], nil, 0.5) or raise Timeout::Error
end

And could be replaced with:

TCPSocket.new("127.0.0.1", 9418, connect_timeout: 0.5)

I am not sure what exception TCPSocket.new should return. Timeout::Error
seems like a reasonable choice...


Files

Updated by naruse (Yui NARUSE) over 12 years ago

  • Status changed from Open to Assigned
  • Assignee set to akr (Akira Tanaka)

Updated by normalperson (Eric Wong) over 12 years ago

Eric Wong wrote:

Proposed API would be:

TCPSocket.new(remote_host, remote_port,
connect_timeout: 0.5, local_host: nil, local_port: nil)

Btw, I'll gladly provide patches + tests if the API is approved or
a better one is suggested :>

Updated by normalperson (Eric Wong) over 12 years ago

Eric Wong wrote:

Proposed API would be:

TCPSocket.new(remote_host, remote_port,
connect_timeout: 0.5, local_host: nil, local_port: nil)

An alternative (or complimentary) API could also be to add a new class
method that only starts the TCP connection, but returns the socket
on EINPROGRESS:

TCPSocket.start(remote_host, remote_port,
local_host=nil, local_port=nil) => tcp_socket

This means the user would have to wait for writability on the socket
returned by this method (e.g. with IO.select).

I've already implemented something similar for kgio:
http://bogomips.org/kgio/Kgio/TCPSocket.html#method-c-start

.. and am of course happy to provide patches for Ruby itself if
ruby-core agrees it's a good API to have :>

--
Eric Wong

Updated by akr (Akira Tanaka) over 12 years ago

2011/7/27 Eric Wong :

Proposed API would be:

TCPSocket.new(remote_host, remote_port,
connect_timeout: 0.5, local_host: nil, local_port: nil)

Or:

TCPSocket.new(remote_host, remote_port, connect_timeout: 0.5)

How about Socket class?
You can implement timeout on Socket.

I think you need full power of socket API,
so you should use low level bindings for socket API i.e. Socket class.

Note that Socket is not so cumbersome since Ruby 1.9.2.

Tanaka Akira

Updated by normalperson (Eric Wong) over 12 years ago

Tanaka Akira wrote:

2011/7/27 Eric Wong :

Proposed API would be:

TCPSocket.new(remote_host, remote_port,
connect_timeout: 0.5, local_host: nil, local_port: nil)

Or:

TCPSocket.new(remote_host, remote_port, connect_timeout: 0.5)

How about Socket class?
You can implement timeout on Socket.

I think you need full power of socket API,
so you should use low level bindings for socket API i.e. Socket class.

Note that Socket is not so cumbersome since Ruby 1.9.2.

Like my original example in the ticket?

 addr = Socket.pack_sockaddr_in(9418, "127.0.0.1")
 s = Socket.new(:AF_INET, :SOCK_STREAM, 0)
 begin
   s.connect_nonblock(addr)
 rescue Errno::EINPROGRESS
   IO.select(nil, [s], nil, 0.5) or raise Timeout::Error
 end

I can only seem to shorten it to the following, is there a better way?
I can't find it looking through ext/socket/...

 addr = Addrinfo.tcp("127.0.0.1", 9418)
 s = Socket.new(:AF_INET, :SOCK_STREAM)
 begin
   s.connect_nonblock(addr)
 rescue Errno::EINPROGRESS
   IO.select(nil, [s], nil, 0.5) or raise Timeout::Error
 end

Anyways I would like to be able to implement open_timeout in net/http
without using threads (with timeout.rb). Also see
http://redmine.ruby-lang.org/issues/5100 for resolv.rb timeouts.

--
Eric Wong

Updated by akr (Akira Tanaka) over 12 years ago

2011/7/29 Eric Wong :

Like my original example in the ticket?

addr = Socket.pack_sockaddr_in(9418, "127.0.0.1")
s = Socket.new(:AF_INET, :SOCK_STREAM, 0)
begin
s.connect_nonblock(addr)
rescue Errno::EINPROGRESS
IO.select(nil, [s], nil, 0.5) or raise Timeout::Error
end

I can only seem to shorten it to the following, is there a better way?
I can't find it looking through ext/socket/...

addr = Addrinfo.tcp("127.0.0.1", 9418)
s = Socket.new(:AF_INET, :SOCK_STREAM)
begin
s.connect_nonblock(addr)
rescue Errno::EINPROGRESS
IO.select(nil, [s], nil, 0.5) or raise Timeout::Error
end

I expected such code in client socket library because
socket library doesn't provide timeout feature (and Timeout exception class).

Anyways I would like to be able to implement open_timeout in net/http
without using threads (with timeout.rb). Also see

I tested net/http using Socket class as follows.
This doesn't cause any problems with test-all.

Index: lib/net/http.rb

--- lib/net/http.rb (revision 32734)
+++ lib/net/http.rb (working copy)
@@ -763,7 +763,7 @@

  def connect
    D "opening connection to #{conn_address()}..."
  •  s = timeout(@open_timeout) { TCPSocket.open(conn_address(),
    

conn_port()) }

  •  s = timeout(@open_timeout) { Socket.tcp(conn_address(), conn_port()) }
     D "opened"
     if use_ssl?
       ssl_parameters = Hash.new
    

I remember multiple IP addresses issue.
The behavior of timeout is not clear if two or more IP addresses are
given for a hostname.

Tanaka Akira

Updated by akr (Akira Tanaka) over 12 years ago

2011/7/30 Tanaka Akira :

I expected such code in client socket library because
socket library doesn't provide timeout feature (and Timeout exception class).

On second thought, Errno::ETIMEDOUT can be a candidate.

I remember multiple IP addresses issue.
The behavior of timeout is not clear if two or more IP addresses are
given for a hostname.

This is still a problem.

Tanaka Akira

Updated by normalperson (Eric Wong) over 12 years ago

Tanaka Akira wrote:

2011/7/30 Tanaka Akira :

I expected such code in client socket library because
socket library doesn't provide timeout feature (and Timeout exception class).

On second thought, Errno::ETIMEDOUT can be a candidate.

OK; sounds good.

I remember multiple IP addresses issue.
The behavior of timeout is not clear if two or more IP addresses are
given for a hostname.

This is still a problem.

I only want this timeout to affect connect() time, DNS timeout can
be separate (Feature #5100). I don't know what to do about multiple
IP addresses, it may be better on a case-by-case basis or until total
time runs out.

Maybe even have an option to do a non-blocking connect() to all IPs
simultaneously and use the first one that returns writability
with IO.select:

This may make some server/network admins unhappy and even be

considered abusive and get clients banned, but it could be useful

in some situations; too

sockets = addresses.map do |addr|
 s = Socket.new(:AF_INET, :SOCK_STREAM)
 begin
   s.connect_nonblock(addr)
 rescue Errno::EINPROGRESS
 end
 s

end
ready = IO.select(nil, sockets, nil, 0.5) or raise Errno::ETIMEDOUT
keep = nil
ready[1].each do |sock|
if keep
sock.close
else
begin
do_something(sock)
keep = sock
rescue
sock.close
end
end
end
do_more_things(keep)

--
Eric Wong

Updated by akr (Akira Tanaka) over 12 years ago

2011/8/2 Eric Wong :

I only want this timeout to affect connect() time, DNS timeout can
be separate (Feature #5100). I don't know what to do about multiple
IP addresses, it may be better on a case-by-case basis or until total
time runs out.

I see.

If N IP address is obtained, N * connect_timeout seconds may be required for
timeout.

Maybe even have an option to do a non-blocking connect() to all IPs
simultaneously and use the first one that returns writability
with IO.select:

Clealy, it should be optional.

Tanaka Akira

Updated by akr (Akira Tanaka) over 12 years ago

I made a pactch for connect_timeout for Socket.tcp.

with timeout:
% time ./ruby -rsocket -e 'Socket.tcp("192.0.2.1", 80, :connect_timeout=>1)'
/home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:48:in rescue in connect_internal': Connection timed out - user specified timeout (Errno::ETIMEDOUT) from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:44:in connect_internal'
from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:108:in connect' from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:277:in block in tcp'
from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:181:in each' from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:181:in foreach'
from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:267:in tcp' from -e:1:in '
./ruby -rsocket -e 'Socket.tcp("192.0.2.1", 80, :connect_timeout=>1)' 0.12s user 0.00s system 10% cpu 1.126 total

without timeout:
% time ./ruby -rsocket -e 'Socket.tcp("192.0.2.1", 80)'
/home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:56:in connect': Connection timed out - connect(2) (Errno::ETIMEDOUT) from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:56:in connect_internal'
from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:108:in connect' from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:277:in block in tcp'
from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:181:in each' from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:181:in foreach'
from /home/akr/ruby/tst1/lib/ruby/1.9.1/socket.rb:267:in tcp' from -e:1:in '
./ruby -rsocket -e 'Socket.tcp("192.0.2.1", 80)' 0.12s user 0.00s system 0% cpu 3:09.51 total

Updated by normalperson (Eric Wong) over 12 years ago

Akira Tanaka wrote:

File socket-tcp-connect-timeout.patch added

Looks good to me, thanks!

Updated by akr (Akira Tanaka) over 12 years ago

I updated the patch for document and argument checking.
I'll commit it later.

Updated by akr (Akira Tanaka) over 12 years ago

  • Status changed from Assigned to Closed
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0