How to make net-ping thread safe?

L

Laurent Julliard

Hi,

I'm using the excellent net-ping gem (v 1.2.2 on Ruby 1.8.6 Linux box)
from Daniel Berger to ping a number of machines on a local area network.

I recently refactored my code to use threads and I'm having troubles
with Net::ping::ICMP. I'm trying to ping several machines, each one in a
separate thread and I'm seing strange things like when one machine is on
all three machines are reported on (although two are off). When I run my
program with one thread only the on/off state shows as expected.

So I'm wondering if the Net::ping::ICMP#ping method is thread safe?
Daniel said he made no attempt to make sure that the code is and
usggested that I post this question here.

The code for the method Net::ping::ICMP#ping is here:
http://www.koders.com/ruby/fidE6256BC790B8AD197544CE26287B5C7D3200E4C8.aspx?s=icmp#L5

(line 61)

Here is also a simplified version of my own code:

-----------------------------------------------

require 'rubygems'
require 'net/ping/icmp'

WORK_STATIONS = ['192.168.1.1', '192.168.1.2', '192.168.1.3']
threads = []
ping_objects = []

WORK_STATIONS.each do |ip|
ping_objects << Net::ping::ICMP.new(ip)
threads << Thread.new(ip, ping_objects.last) do |ip, p|
puts "Monitoring #{ip}..."
loop do
puts "#{ip} is #{p.ping ? 'on' : 'off'}"
sleep 2
end
end
end

threads.each { |th| th.join }

------------------------------------------------

Any advice either to change my own script or improve the net-ping code
is welcome.

Thanks for your help!

Laurent
 
J

Joel VanderWerf

Laurent said:
Hi,

I'm using the excellent net-ping gem (v 1.2.2 on Ruby 1.8.6 Linux box)
from Daniel Berger to ping a number of machines on a local area network.

I recently refactored my code to use threads and I'm having troubles
with Net::ping::ICMP. I'm trying to ping several machines, each one in a
separate thread and I'm seing strange things like when one machine is on
all three machines are reported on (although two are off). When I run my
program with one thread only the on/off state shows as expected.

So I'm wondering if the Net::ping::ICMP#ping method is thread safe?
Daniel said he made no attempt to make sure that the code is and
usggested that I post this question here.

The code for the method Net::ping::ICMP#ping is here:
http://www.koders.com/ruby/fidE6256BC790B8AD197544CE26287B5C7D3200E4C8.aspx?s=icmp#L5


(line 61)

Here is also a simplified version of my own code:

-----------------------------------------------

require 'rubygems'
require 'net/ping/icmp'

WORK_STATIONS = ['192.168.1.1', '192.168.1.2', '192.168.1.3']
threads = []
ping_objects = []

WORK_STATIONS.each do |ip|
ping_objects << Net::ping::ICMP.new(ip)
threads << Thread.new(ip, ping_objects.last) do |ip, p|
puts "Monitoring #{ip}..."
loop do
puts "#{ip} is #{p.ping ? 'on' : 'off'}"
sleep 2
end
end
end

threads.each { |th| th.join }

------------------------------------------------

Any advice either to change my own script or improve the net-ping code
is welcome.

Thanks for your help!

Laurent
Click to expand...

In icmp.rb, the problem seems to be that it uses process ID as the ICMP
packet ID, so it is impossible to tell which ECHO REPLY packet
corresponds to which request (if multiple requests come from one process).

Quoting http://tools.ietf.org/html/rfc792:

The identifier and sequence number may be used by the echo sender
to aid in matching the replies with the requests. For example,
the identifier might be used like a port in TCP or UDP to identify
a session, and the sequence number might be incremented on each
request sent. The destination returns these same values in the
reply.

Maybe it is standard practice to use pid, so that multiple ICMP clients
(e.g. /bin/ping) never step on each other. In that case, I guess icmp.rb
could fork{sleep} for each Ping instance and use the child pid as a
conflict-free identifier.[1]

However, watching tcpdump as /bin/ping is running on linux, it seems
that the ID might be a local port rather than process id. Maybe ping is
just binding a socket to 0 to ask the OS for a unique port.

Ruby's icmp should do whatever /bin/ping does.

You could work around this by filtering the response by sender host,
*if* you assume that different threads never ping the same host.

Or you could just shell out to ping (which is what I always do).

[1] http://www.ping127001.com/pingpage/ping.html uses PID.
 
J

Joel VanderWerf

Joel said:
In icmp.rb, the problem seems to be that it uses process ID as the ICMP
packet ID, so it is impossible to tell which ECHO REPLY packet
corresponds to which request (if multiple requests come from one process).
Click to expand...

On second thought, the problem could be solved in icmp.rb by using the
sequence number to separate requests, and keeping the sequence number in
an ICMP class var instead of instance vars. Here's a patch that seems to
make your original code work correctly (detecting some hosts up and some
down).

---
/usr/local/lib/ruby/gems/1.8/gems/net-ping-1.2.2/lib/net/ping/icmp.rb
2008-01-23 19:46:14.000000000 -0800
+++ net/ping/icmp.rb 2008-03-15 13:09:55.000000000 -0700
@@ -24,7 +24,6 @@
def initialize(host=nil, port=nil, timeout=5)
raise 'requires root privileges' if Process.euid > 0

- @seq = 0
@bind_port = 0
@bind_host = nil
@data_size = 56
@@ -37,6 +36,11 @@
super(host, port, timeout)
@port = nil # This value is not used in ICMP pings.
end
+
+ @seq = 0
+ def self.next_seq
+ @seq = (@seq + 1) % 65536
+ end

# Sets the number of bytes sent in the ping method.
#
@@ -73,14 +77,14 @@
socket.bind(saddr)
end

- @seq = (@seq + 1) % 65536
+ seq = ICMP.next_seq
pstring = 'C2 n3 A' << @data_size.to_s
timeout = @timeout

checksum = 0
- msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, @seq,
@data].pack(pstring)
+ msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, seq,
@data].pack(pstring)
checksum = checksum(msg)
- msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, @seq,
@data].pack(pstring)
+ msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, seq,
@data].pack(pstring)

start_time = Time.now

@@ -101,7 +105,7 @@
end

pid = nil
- seq = nil
+ rcv_seq = nil

data, sender = socket.recvfrom(1500)
port, host = Socket.unpack_sockaddr_in(sender)
@@ -110,15 +114,15 @@
case type
when ICMP_ECHOREPLY
if data.length >= 28
- pid, seq = data[24, 4].unpack('n3')
+ pid, rcv_seq = data[24, 4].unpack('n3')
end
else
if data.length > 56
- pid, seq = data[52, 4].unpack('n3')
+ pid, rcv_seq = data[52, 4].unpack('n3')
end
end

- if pid == @pid && seq == @seq && type == ICMP_ECHOREPLY
+ if pid == @pid && rcv_seq == seq && type == ICMP_ECHOREPLY
bool = true
end
}
 
J

Joel VanderWerf

Forgot to make the previous patch thread safe:

--- net/ping/icmp.rb.bck 2008-03-15 13:19:42.000000000 -0700
+++ net/ping/icmp.rb 2008-03-15 13:19:42.000000000 -0700
@@ -1,5 +1,6 @@
$LOAD_PATH.unshift File.dirname(__FILE__)
require 'ping'
+require 'thread'

module Net
class Ping::ICMP < Ping
@@ -38,8 +39,11 @@
end

@seq = 0
+ @seq_mutex = Mutex.new
def self.next_seq
- @seq = (@seq + 1) % 65536
+ @seq_mutex.synchronize do
+ @seq = (@seq + 1) % 65536
+ end
end

# Sets the number of bytes sent in the ping method.
 
D

Daniel Berger

Hi,

I'm using the excellent net-ping gem (v 1.2.2 on Ruby 1.8.6 Linux box)
from Daniel Berger to ping a number of machines on a local area network.

I recently refactored my code to use threads and I'm having troubles
with Net::ping::ICMP. I'm trying to ping several machines, each one in a
separate thread and I'm seing strange things like when one machine is on
all three machines are reported on (although two are off). When I run my
program with one thread only the on/off state shows as expected.

So I'm wondering if the Net::ping::ICMP#ping method is thread safe?
Click to expand...

<snip>

Hi Laurent,

Please apply Joel's patches and let me know how it works. If all is
well I'll apply the patches and put out another release.

BTW, thanks Joel!

Regards,

Dan
 
L

Laurent Julliard

Joel said:
On second thought, the problem could be solved in icmp.rb by using the
sequence number to separate requests, and keeping the sequence number in
an ICMP class var instead of instance vars. Here's a patch that seems to
make your original code work correctly (detecting some hosts up and some
down).
Click to expand...

Joel,

Fist of all thanks for catching this!

Your recommendation is to turn the sequence number into a class variable
so shouldn't the sequence number really be named @@seq rather than @seq
if we want the sequence number to be unique across multiple instances of
ICMP objects?

And if this is so, one of the consequence is that for a given ICMP
instance chances are that the seq field in the icmp packet will not
contain consecutive values. I don't know if this is a problem wrt to the
ICMP specifications or not.

Don't you think an alternative would be not to use the PID as the packet
identifier but rather a class variable of our own that we would
increment each time a new ICMP instance is created?

Thanks again for your comments.

Laurent


---
/usr/local/lib/ruby/gems/1.8/gems/net-ping-1.2.2/lib/net/ping/icmp.rb
2008-01-23 19:46:14.000000000 -0800
+++ net/ping/icmp.rb 2008-03-15 13:09:55.000000000 -0700
@@ -24,7 +24,6 @@
def initialize(host=nil, port=nil, timeout=5)
raise 'requires root privileges' if Process.euid > 0

- @seq = 0
@bind_port = 0
@bind_host = nil
@data_size = 56
@@ -37,6 +36,11 @@
super(host, port, timeout)
@port = nil # This value is not used in ICMP pings.
end
+
+ @seq = 0
+ def self.next_seq
+ @seq = (@seq + 1) % 65536
+ end

# Sets the number of bytes sent in the ping method.
#
@@ -73,14 +77,14 @@
socket.bind(saddr)
end

- @seq = (@seq + 1) % 65536
+ seq = ICMP.next_seq
pstring = 'C2 n3 A' << @data_size.to_s
timeout = @timeout

checksum = 0
- msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, @seq,
@data].pack(pstring)
+ msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, seq,
@data].pack(pstring)
checksum = checksum(msg)
- msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, @seq,
@data].pack(pstring)
+ msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, seq,
@data].pack(pstring)

start_time = Time.now

@@ -101,7 +105,7 @@
end

pid = nil
- seq = nil
+ rcv_seq = nil

data, sender = socket.recvfrom(1500)
port, host = Socket.unpack_sockaddr_in(sender)
@@ -110,15 +114,15 @@
case type
when ICMP_ECHOREPLY
if data.length >= 28
- pid, seq = data[24, 4].unpack('n3')
+ pid, rcv_seq = data[24, 4].unpack('n3')
end
else
if data.length > 56
- pid, seq = data[52, 4].unpack('n3')
+ pid, rcv_seq = data[52, 4].unpack('n3')
end
end

- if pid == @pid && seq == @seq && type == ICMP_ECHOREPLY
+ if pid == @pid && rcv_seq == seq && type == ICMP_ECHOREPLY
bool = true
end
}
Click to expand...
 
J

Jean-François Trân

2008/3/16 said:
Your recommendation is to turn the sequence number into a class
variable so shouldn't the sequence number really be named @@seq
rather than @seq if we want the sequence number to be unique across
multiple instances of ICMP objects?
Click to expand...

If I am correct, Joel is using a ICMP class instance variable instead
of a class variable. So in the patch, @seq is not a var of an ICMP
object but an instance variable of ICMP class (as an object :) ).

-- Jean-Fran=E7ois.
 
D

Daniel Berger

Laurent said:
Hi,

I'm using the excellent net-ping gem (v 1.2.2 on Ruby 1.8.6 Linux box)
from Daniel Berger to ping a number of machines on a local area network.

I recently refactored my code to use threads and I'm having troubles
with Net::ping::ICMP. I'm trying to ping several machines, each one in a
separate thread and I'm seing strange things like when one machine is on
all three machines are reported on (although two are off). When I run my
program with one thread only the on/off state shows as expected.

So I'm wondering if the Net::ping::ICMP#ping method is thread safe?
Daniel said he made no attempt to make sure that the code is and
usggested that I post this question here.

The code for the method Net::ping::ICMP#ping is here:
http://www.koders.com/ruby/fidE6256BC790B8AD197544CE26287B5C7D3200E4C8.aspx?s=icmp#L5


(line 61)

Here is also a simplified version of my own code:

-----------------------------------------------

require 'rubygems'
require 'net/ping/icmp'

WORK_STATIONS = ['192.168.1.1', '192.168.1.2', '192.168.1.3']
threads = []
ping_objects = []

WORK_STATIONS.each do |ip|
ping_objects << Net::ping::ICMP.new(ip)
threads << Thread.new(ip, ping_objects.last) do |ip, p|
puts "Monitoring #{ip}..."
loop do
puts "#{ip} is #{p.ping ? 'on' : 'off'}"
sleep 2
end
end
end

threads.each { |th| th.join }
Click to expand...

Very late reply here.

Odd. When I ran nearly identical code it worked as expected. However,
when applied Joel's patch it didn't work right - it reported all hosts
as off. Not sure why.

That was on OS X 10.4.9 with Ruby 1.8.6-368 btw.

Regards,

Dan
 
J

Joel VanderWerf

Daniel Berger wrote:
...
Very late reply here.
Click to expand...

Hi, Daniel,

Found the original and dusted it off...

http://blade.nagaokaut.ac.jp/cgi-bin/vframe.rb/ruby/ruby-talk/294693?294583-296534
Odd. When I ran nearly identical code it worked as expected. However,
when applied Joel's patch it didn't work right - it reported all hosts
as off. Not sure why.
Click to expand...

Will take a look on linux first. (This is not code I've used, but I'm
curious...)
That was on OS X 10.4.9 with Ruby 1.8.6-368 btw.
Click to expand...

I'll try there too.

What were the target hosts? OS X, too?
 
L

Leo Stern

Joel said:
Forgot to make the previous patch thread safe:

--- net/ping/icmp.rb.bck 2008-03-15 13:19:42.000000000 -0700
+++ net/ping/icmp.rb 2008-03-15 13:19:42.000000000 -0700
@@ -1,5 +1,6 @@
$LOAD_PATH.unshift File.dirname(__FILE__)
require 'ping'
+require 'thread'
Click to expand...



This does not work.
 
L

Leo Stern

dog = PingerController.new
Monitoring 173.45.228.218...Monitoring 173.45.235.145...Monitoring
192.168.1.3...
Monitoring 127.0.0.1...
Monitoring 24.121.214.12...


173.45.235.145 is off173.45.228.218 is off192.168.1.3 is off127.0.0.1 is
off


24.121.214.12 is off

173.45.228.218 is off24.121.214.12 is off127.0.0.1 is off192.168.1.3 is
off173.45.235.145 is off




24.121.214.12 is off127.0.0.1 is off192.168.1.3 is off173.45.235.145 is
off173.45.228.218 is off




127.0.0.1 is off192.168.1.3 is off173.45.235.145 is off173.45.228.218 is
off24.121.214.12 is off




173.45.228.218 is off24.121.214.12 is off127.0.0.1 is off192.168.1.3 is
off173.45.235.145 is off




^X^CIRB::Abort: abort then interrupt!!
from /usr/local/lib/ruby/1.8/irb.rb:81:in `irb_abort'
from /usr/local/lib/ruby/1.8/irb.rb:247:in `signal_handle'
from /usr/local/lib/ruby/1.8/irb.rb:66:in `start'
from
/Users/lsternlicht/pinger/app/controllers/pinger_controller.rb:32:in
`call'
from
/Users/lsternlicht/pinger/app/controllers/pinger_controller.rb:32:in
`join'
from
/Users/lsternlicht/pinger/app/controllers/pinger_controller.rb:32:in
`pingHostThreaded'
from
/Users/lsternlicht/pinger/app/controllers/pinger_controller.rb:32:in
`each'
from
/Users/lsternlicht/pinger/app/controllers/pinger_controller.rb:32:in
`pingHostThreaded'
from (irb):2=> #<Net::ping::ICMP:0x102301148 @duration=nil, @timeout=5, @port=nil,
@host="173.45.235.145", @bind_port=0, @warning=nil, @pid=26254,
@exception=nil,
@data="\000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023\024\025\026\027\030\031\032\e\034\035\036\037
!\"\#$%&'()*+,-./012345678", @bind_host=nil, @data_size=56>24.121.214.12 is off127.0.0.1 is off192.168.1.3 is off173.45.235.145 is
off173.45.228.218 is off=> #<Net::ping::ICMP:0x1022c2f10 @duration=nil,
@timeout=5, @port=nil, @host="173.45.228.218", @bind_port=0,
@warning=nil, @pid=26254, @exception=nil,
@data="\000\001\002\003\004\005\006\a\b\t\n\v\f\r\016\017\020\021\022\023\024\025\026\027\030\031\032\e\034\035\036\037
!\"\#$%&'()*+,-./012345678", @bind_host=nil, @data_size=56>

anyone have any ideas about the last object instantiation result?
 
L

Leo Stern

The gem is not thread-safe. I am using ruby 1.8 and Mac os X 10.6. I
think Joel has the right idea but I got the same "off" result as Daniel.
 
L

Leo Stern

Jean-François Trân said:
If I am correct, Joel is using a ICMP class instance variable instead
of a class variable. So in the patch, @seq is not a var of an ICMP
object but an instance variable of ICMP class (as an object :) ).

-- Jean-Fran�ois.
Click to expand...

Why?
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Net-Ping ICMP issues (Windows) 0
[ANN] net-ping 1.2.0 1
Thread-safe FileUtils.cd 4
psphere: how to make thread safe 3
Net::Imap thread safety 0
How to make XSUBs thread-safe? xsubpp switches? 3
Thread-safe reference counts. 117
Parallel ping problems python puzzler 3

Members online

Total: 18 (members: 3, guests: 15)
Robots: 299

Forum statistics

Threads
473,755
Messages
2,569,535
Members
45,007
Latest member
obedient dusk

Latest Threads

Top