This repository has been archived by the owner. It is now read-only.

Threaded ICMP with tests, response_data, external runs with ruby 1.8.7, plus .... #30

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
3 participants
@ianheggie
Contributor

ianheggie commented Mar 24, 2014

  • Add threading support for ICMP and test as root as per https://www.ruby-forum.com/topic/146116;
  • Support external with ruby 1.8.7 where the external command returns "N% [packet ]loss";
  • Add response_data for most types (icmp, wmi not done) Based on rigor/master;
  • Exclude tests that don't work with non mri rubies;
    • jruby: Excludes UDP, ICMP, TCP
    • rbx: Excludes ICMP
  • Note in README that ICMP doesn't resolve hostnames;
  • Use 127.0.0.1 rather than 192.168.0.1;

conarro and others added some commits Jun 22, 2013

Add threading support for ICMP and test as root
as per https://www.ruby-forum.com/topic/146116;

Support external with ruby 1.8.7 where the external
command returns "N% [packet ]loss";

Add response_data for most types (icmp, wmi not done)
Based on rigor/master;

Exclude tests that don't work with non mri rubies;

Note in README that ICMP doesn't resolve hostnames;

Use 127.0.0.1 rather than 192.168.0.1;
Merge remote-tracking branch 'rigor/master'
Conflicts:
	Gemfile
	lib/net/ping/tcp.rb
	net-ping.gemspec
@ianheggie

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 24, 2014

Contributor

Note this fixes bug #22

Contributor

ianheggie commented Mar 24, 2014

Note this fixes bug #22

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Mar 24, 2014

It doesn't?

It doesn't?

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 25, 2014

Owner

I took these changes from kconarro14's fork - on reflection if we get to this point then the packet has been returned by some service on the server being pinged, and I expect most udp services would return something different to what was sent (except for echo on port 7).

I think it would be reasonable to make the test:

ECHO_PORT = 7

if @response_data && (@response_data == @data || @port != ECHO_PORT)

What do you think?

Owner

ianheggie replied Mar 25, 2014

I took these changes from kconarro14's fork - on reflection if we get to this point then the packet has been returned by some service on the server being pinged, and I expect most udp services would return something different to what was sent (except for echo on port 7).

I think it would be reasonable to make the test:

ECHO_PORT = 7

if @response_data && (@response_data == @data || @port != ECHO_PORT)

What do you think?

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 25, 2014

Owner

Have changed code accordingly. I am not sure of what dns does with ill formatted packets, if it sends back a "huh?" style message or it ignores them. Do you have any example udp services you have used we could examine for what they normally send back? (Most of the ones I have used in the past are normally disabled now days)

Owner

ianheggie replied Mar 25, 2014

Have changed code accordingly. I am not sure of what dns does with ill formatted packets, if it sends back a "huh?" style message or it ignores them. Do you have any example udp services you have used we could examine for what they normally send back? (Most of the ones I have used in the past are normally disabled now days)

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Mar 24, 2014

I know people like pry, but it seems unnecessary to add to the Gemfile.

I know people like pry, but it seems unnecessary to add to the Gemfile.

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 25, 2014

Owner

Opps ... that escaped my notice when I redid the original merge from kconarro14, and then squashed just my commits. Have fixed.

Owner

ianheggie replied Mar 25, 2014

Opps ... that escaped my notice when I redid the original merge from kconarro14, and then squashed just my commits. Have fixed.

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Mar 24, 2014

Hm, dunno about this one.

Hm, dunno about this one.

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 25, 2014

Owner

Removed from gemspec

Owner

ianheggie replied Mar 25, 2014

Removed from gemspec

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Mar 24, 2014

Regarding the ICMP pings, I'm not sure what you mean by only resolves IP addresses. I can ping ruby-lang.org with it fine. Also, I think your approach to thread safe code was tried before and failed.

Lastly, I think my Timeout wrapper is probably unnecessary, and may even be counterproductive.

Regarding the ICMP pings, I'm not sure what you mean by only resolves IP addresses. I can ping ruby-lang.org with it fine. Also, I think your approach to thread safe code was tried before and failed.

Lastly, I think my Timeout wrapper is probably unnecessary, and may even be counterproductive.

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 25, 2014

Owner

Regarding the ICMP pings, I'm not sure what you mean by only resolves IP addresses. I can ping ruby-lang.org with it fine.

I got this error when trying to ping google.com:
https://travis-ci.org/ianheggie/net-ping/jobs/21402407
/home/travis/build/ianheggie/net-ping/lib/net/ping/icmp.rb:117:in `send': Address family not supported by protocol - send(2) (Errno::EAFNOSUPPORT)

Also, I think your approach to thread safe code was tried before and failed.

I added tests, and the tests passed after adding in the code (it was the fix suggested by the link) - do you have an example where it fails? I could then add it to the test and then see if I can make the code work with the enhanced test.

Possibly just mark it in the README as "fix applied from (link), but only tested under Linux - feedback welcome" ?

Lastly, I think my Timeout wrapper is probably unnecessary, and may even be counterproductive.

The issue with JRuby and UDP, Timeout has been commented in the link I attached - it appears to be an issue that isn't simple to solve in JRuby and probably wont be for some time. Therefore I concluded it wasn't your use of Timeout, but simply that JRuby doesn't allow the recvfrom to be interrupted.

Personally I prefer cautious programming, so I would leave the Timeout wrapper in, unless we can use select in the UDP case to detect a packet has arrived, in which case swapping to that would make sense.

Owner

ianheggie replied Mar 25, 2014

Regarding the ICMP pings, I'm not sure what you mean by only resolves IP addresses. I can ping ruby-lang.org with it fine.

I got this error when trying to ping google.com:
https://travis-ci.org/ianheggie/net-ping/jobs/21402407
/home/travis/build/ianheggie/net-ping/lib/net/ping/icmp.rb:117:in `send': Address family not supported by protocol - send(2) (Errno::EAFNOSUPPORT)

Also, I think your approach to thread safe code was tried before and failed.

I added tests, and the tests passed after adding in the code (it was the fix suggested by the link) - do you have an example where it fails? I could then add it to the test and then see if I can make the code work with the enhanced test.

Possibly just mark it in the README as "fix applied from (link), but only tested under Linux - feedback welcome" ?

Lastly, I think my Timeout wrapper is probably unnecessary, and may even be counterproductive.

The issue with JRuby and UDP, Timeout has been commented in the link I attached - it appears to be an issue that isn't simple to solve in JRuby and probably wont be for some time. Therefore I concluded it wasn't your use of Timeout, but simply that JRuby doesn't allow the recvfrom to be interrupted.

Personally I prefer cautious programming, so I would leave the Timeout wrapper in, unless we can use select in the UDP case to detect a packet has arrived, in which case swapping to that would make sense.

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Mar 24, 2014

Owner

Overall I think you've done some good work here, but I much prefer smaller chunks at a time to deal with. Any chance you can break these down into more manageable chunks for me to review/comment/merge?

Owner

djberg96 commented Mar 24, 2014

Overall I think you've done some good work here, but I much prefer smaller chunks at a time to deal with. Any chance you can break these down into more manageable chunks for me to review/comment/merge?

@ianheggie

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 25, 2014

Contributor

I had worked on several aspects in different branches, around 50 commits ... I felt that it was enough of a tangle that I squashed it together before submitting, to hopefully make it easier to review ...

BUT I could have a look at doing the split next weekend ... would you be happy with the following split?

  1. Add tests
    then based off that
  2. Response_data
    then based off that
  3. ICMP thread support
  4. External ping for ruby 1.8.7

(This would make the branches able to be merged without conflict - branch 3 and 4 use response data, so it makes sense for response_data to be 2nd)

Contributor

ianheggie commented Mar 25, 2014

I had worked on several aspects in different branches, around 50 commits ... I felt that it was enough of a tangle that I squashed it together before submitting, to hopefully make it easier to review ...

BUT I could have a look at doing the split next weekend ... would you be happy with the following split?

  1. Add tests
    then based off that
  2. Response_data
    then based off that
  3. ICMP thread support
  4. External ping for ruby 1.8.7

(This would make the branches able to be merged without conflict - branch 3 and 4 use response data, so it makes sense for response_data to be 2nd)

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Mar 25, 2014

Owner

Let's keep it simple and just start with adding the @DaTa accessor and go from there.

Owner

djberg96 commented Mar 25, 2014

Let's keep it simple and just start with adding the @DaTa accessor and go from there.

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Mar 25, 2014

Owner

BTW, I dropped 1.8 support deliberately. Also, regarding the Timeout, I was referring to ICMP pings, since IO::select already handles timeouts.

Owner

djberg96 commented Mar 25, 2014

BTW, I dropped 1.8 support deliberately. Also, regarding the Timeout, I was referring to ICMP pings, since IO::select already handles timeouts.

@ianheggie

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 26, 2014

Contributor

Let's keep it simple and just start with adding the @DaTa accessor and go from there.

Do you mind if I do "add travis-ci testing" first? (Since I use travis-ci to confirm the tests pass on various rubies, Github links to the travis-ci results automatically, and I prefer TDD).

BTW, I dropped 1.8 support deliberately.

I'll keep a ruby_18 branch on my fork, and if you don't mind add a comment in README accordingly. (then it is up to me what I backport from any future changes)

Also, regarding the Timeout, I was referring to ICMP pings, since IO::select already handles timeouts.

Yes, that is what I meant as well.

Contributor

ianheggie commented Mar 26, 2014

Let's keep it simple and just start with adding the @DaTa accessor and go from there.

Do you mind if I do "add travis-ci testing" first? (Since I use travis-ci to confirm the tests pass on various rubies, Github links to the travis-ci results automatically, and I prefer TDD).

BTW, I dropped 1.8 support deliberately.

I'll keep a ruby_18 branch on my fork, and if you don't mind add a comment in README accordingly. (then it is up to me what I backport from any future changes)

Also, regarding the Timeout, I was referring to ICMP pings, since IO::select already handles timeouts.

Yes, that is what I meant as well.

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Mar 26, 2014

Owner

Ok, let's start with Travis CI.

Owner

djberg96 commented Mar 26, 2014

Ok, let's start with Travis CI.

@ianheggie

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Mar 26, 2014

Contributor

Closed pull request since it will be split.

Contributor

ianheggie commented Mar 26, 2014

Closed pull request since it will be split.

@ianheggie ianheggie closed this Mar 26, 2014

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Apr 3, 2014

Owner

BTW, I tried that ICMP code again. It did not work for me on OSX:

>sudo ruby icmp.rb 
Waiting for threads
check ping 8.8.9.9 returned false, with exception: timeout, which IS as expected
check ping 8.8.4.4 returned false, with exception: timeout, which is NOT as expected
Owner

djberg96 commented Apr 3, 2014

BTW, I tried that ICMP code again. It did not work for me on OSX:

>sudo ruby icmp.rb 
Waiting for threads
check ping 8.8.9.9 returned false, with exception: timeout, which IS as expected
check ping 8.8.4.4 returned false, with exception: timeout, which is NOT as expected
@ianheggie

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Apr 4, 2014

Contributor
Thanks for letting me know, I don't have OS/X myself ... this sort
of network stuff seems to be one of the most variable things between
platforms.
Have you any thoughts on how to fix the icmp stuff to work with
threads?
I suppose I better run up tests for the other types with threads to
make sure they are thread safe as well.
IanOn 04/04/14 03:59, Daniel Berger wrote:

  BTW, I tried that ICMP code again. It did not work for me on
    OSX:
  >sudo ruby icmp.rb 

Waiting for threads
check ping 8.8.9.9 returned false, with exception: timeout, which IS as expected
check ping 8.8.4.4 returned false, with exception: timeout, which is NOT as expected

  —
    Reply to this email directly or view
      it on GitHub.

-- 

Regards,
Ian Heggie

Heggie Enterprises Pty Ltd
email: ian@heggie.biz
mobile: 0425 856 328
skype: ian.heggie or (03) 9005 6328

Contributor

ianheggie commented Apr 4, 2014

Thanks for letting me know, I don't have OS/X myself ... this sort
of network stuff seems to be one of the most variable things between
platforms.
Have you any thoughts on how to fix the icmp stuff to work with
threads?
I suppose I better run up tests for the other types with threads to
make sure they are thread safe as well.
IanOn 04/04/14 03:59, Daniel Berger wrote:

  BTW, I tried that ICMP code again. It did not work for me on
    OSX:
  >sudo ruby icmp.rb 

Waiting for threads
check ping 8.8.9.9 returned false, with exception: timeout, which IS as expected
check ping 8.8.4.4 returned false, with exception: timeout, which is NOT as expected

  —
    Reply to this email directly or view
      it on GitHub.

-- 

Regards,
Ian Heggie

Heggie Enterprises Pty Ltd
email: ian@heggie.biz
mobile: 0425 856 328
skype: ian.heggie or (03) 9005 6328

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Apr 4, 2014

Owner

Actually, I tried it on my Ubuntu VM and got failures as well. Unless I've missed something (you can see my icmp branch), I think the fact that it works for you may be coincidence.

I'm not sure how to solve it. Searching "thread safe icmp" just leads back to the original discussion in most cases. I tried explicitly binding the socket, but that had no effect. At this point I'm just slapping mutexes everywhere to see what happens. I guess this is why Rust is an attractive alternative. ;)

Owner

djberg96 commented Apr 4, 2014

Actually, I tried it on my Ubuntu VM and got failures as well. Unless I've missed something (you can see my icmp branch), I think the fact that it works for you may be coincidence.

I'm not sure how to solve it. Searching "thread safe icmp" just leads back to the original discussion in most cases. I tried explicitly binding the socket, but that had no effect. At this point I'm just slapping mutexes everywhere to see what happens. I guess this is why Rust is an attractive alternative. ;)

@ianheggie

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Apr 4, 2014

Contributor
That is interesting ... it may mean the test isn't thorough enough
if it passes some places and not others.
I will have a glance at your icmp branch.
Slap enough mutexes in and you may end up serialising it (and thus
making it "work") GRINNING.
Anyway, if you are having a go at it, I will leave that to you and
look at why the http ping with service_check on thinks some ports
are open when they are not. 
IanOn 04/04/14 11:24, Daniel Berger wrote:

  Actually, I tried it on my Ubuntu VM and got failures as well.
    Unless I've missed something (you can see my icmp branch), I
    think the fact that it works for you may be coincidence.
  I'm not sure how to solve it. Searching "thread safe icmp" just
    leads back to the original discussion in most cases. I tried
    explicitly binding the socket, but that had no effect. At this
    point I'm just slapping mutexes everywhere to see what happens.
    I guess this is why Rust is an attractive alternative. ;)
  —
    Reply to this email directly or view
      it on GitHub.

-- 

Regards,
Ian Heggie

Heggie Enterprises Pty Ltd
email: ian@heggie.biz
mobile: 0425 856 328
skype: ian.heggie or (03) 9005 6328

Contributor

ianheggie commented Apr 4, 2014

That is interesting ... it may mean the test isn't thorough enough
if it passes some places and not others.
I will have a glance at your icmp branch.
Slap enough mutexes in and you may end up serialising it (and thus
making it "work") GRINNING.
Anyway, if you are having a go at it, I will leave that to you and
look at why the http ping with service_check on thinks some ports
are open when they are not. 
IanOn 04/04/14 11:24, Daniel Berger wrote:

  Actually, I tried it on my Ubuntu VM and got failures as well.
    Unless I've missed something (you can see my icmp branch), I
    think the fact that it works for you may be coincidence.
  I'm not sure how to solve it. Searching "thread safe icmp" just
    leads back to the original discussion in most cases. I tried
    explicitly binding the socket, but that had no effect. At this
    point I'm just slapping mutexes everywhere to see what happens.
    I guess this is why Rust is an attractive alternative. ;)
  —
    Reply to this email directly or view
      it on GitHub.

-- 

Regards,
Ian Heggie

Heggie Enterprises Pty Ltd
email: ian@heggie.biz
mobile: 0425 856 328
skype: ian.heggie or (03) 9005 6328

@ianheggie

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Apr 4, 2014

Contributor
Looks promising ... 
IanOn 04/04/14 12:59, Daniel Berger wrote:

  Perhaps this is the key: https://stackoverflow.com/questions/8888880/python-icmp-ping-implementation-when-pinging-multiple-ips-from-threads
  —
    Reply to this email directly or view
      it on GitHub.

-- 

Regards,
Ian Heggie

Heggie Enterprises Pty Ltd
email: ian@heggie.biz
mobile: 0425 856 328
skype: ian.heggie or (03) 9005 6328

Contributor

ianheggie commented Apr 4, 2014

Looks promising ... 
IanOn 04/04/14 12:59, Daniel Berger wrote:

  Perhaps this is the key: https://stackoverflow.com/questions/8888880/python-icmp-ping-implementation-when-pinging-multiple-ips-from-threads
  —
    Reply to this email directly or view
      it on GitHub.

-- 

Regards,
Ian Heggie

Heggie Enterprises Pty Ltd
email: ian@heggie.biz
mobile: 0425 856 328
skype: ian.heggie or (03) 9005 6328

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Apr 4, 2014

Owner

Is there any downside to replacing Process.pid with Thread.current.object_id? Seems to work with your tests (except 172.0.0.2, which I would expect to fail on my system, as it does from the command line).

Owner

djberg96 commented Apr 4, 2014

Is there any downside to replacing Process.pid with Thread.current.object_id? Seems to work with your tests (except 172.0.0.2, which I would expect to fail on my system, as it does from the command line).

@ianheggie

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Apr 4, 2014

Contributor
Mmm.. I remember in the link you posted someone commented a
combination of the two was better so if two processes where pinging,
you are less likely to get a conflict where the pack is
misidentified. (I remember someone said something about XOR'ing ...
if practical it would make sense to append both process id and
thread if it fits then there should be no overlap rather than
xoring).
I suspect we should upgrade the test we have to run three copies of
the test (so three process id's), and have each process id run three
threads (so three thread ids, typically will be identical across the
different processes), and each thread ping all the hosts (currently
each host is only pinged by one thread) - I suspect that should test
the fix well enough without having to go all the way to generating
UUID's to assure uniqueness.  
IanOn 04/04/14 14:24, Daniel Berger wrote:

  Is there any downside to replacing Process.pid with
    Thread.current.object_id? Seems to work with your tests (except
    172.0.0.2, which I would expect to fail on my system, as it does
    from the command line).
  —
    Reply to this email directly or view
      it on GitHub.

-- 

Regards,
Ian Heggie

Heggie Enterprises Pty Ltd
email: ian@heggie.biz
mobile: 0425 856 328
skype: ian.heggie or (03) 9005 6328

Contributor

ianheggie commented Apr 4, 2014

Mmm.. I remember in the link you posted someone commented a
combination of the two was better so if two processes where pinging,
you are less likely to get a conflict where the pack is
misidentified. (I remember someone said something about XOR'ing ...
if practical it would make sense to append both process id and
thread if it fits then there should be no overlap rather than
xoring).
I suspect we should upgrade the test we have to run three copies of
the test (so three process id's), and have each process id run three
threads (so three thread ids, typically will be identical across the
different processes), and each thread ping all the hosts (currently
each host is only pinged by one thread) - I suspect that should test
the fix well enough without having to go all the way to generating
UUID's to assure uniqueness.  
IanOn 04/04/14 14:24, Daniel Berger wrote:

  Is there any downside to replacing Process.pid with
    Thread.current.object_id? Seems to work with your tests (except
    172.0.0.2, which I would expect to fail on my system, as it does
    from the command line).
  —
    Reply to this email directly or view
      it on GitHub.

-- 

Regards,
Ian Heggie

Heggie Enterprises Pty Ltd
email: ian@heggie.biz
mobile: 0425 856 328
skype: ian.heggie or (03) 9005 6328

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Apr 4, 2014

Owner

Ok, I've XOR'ed the current thread id with the current pid. I guess there's a theoretical clash, but yeah, should be ok. I've pushed this out to master.

Now I'm just seeing if there's some sort of wrapper/binding to RockSaw for JRuby.

Owner

djberg96 commented Apr 4, 2014

Ok, I've XOR'ed the current thread id with the current pid. I guess there's a theoretical clash, but yeah, should be ok. I've pushed this out to master.

Now I'm just seeing if there's some sort of wrapper/binding to RockSaw for JRuby.

@djberg96

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Apr 4, 2014

This must be against an older version, as I ditched TCPSocket.

This must be against an older version, as I ditched TCPSocket.

This comment has been minimized.

Show comment Hide comment
@ianheggie

ianheggie Apr 4, 2014

Owner

It is branched from 3ccaee9

Author: Daniel Berger djberg96@gmail.com 2014-02-26 13:26:38
Committer: Daniel Berger djberg96@gmail.com 2014-02-26 13:26:38
Parent: fecd90d (Added author to README.)
Follows: net-ping-1.7.2

So I presume you changed the TCPSocket after that.

Ian

Owner

ianheggie replied Apr 4, 2014

It is branched from 3ccaee9

Author: Daniel Berger djberg96@gmail.com 2014-02-26 13:26:38
Committer: Daniel Berger djberg96@gmail.com 2014-02-26 13:26:38
Parent: fecd90d (Added author to README.)
Follows: net-ping-1.7.2

So I presume you changed the TCPSocket after that.

Ian

This comment has been minimized.

Show comment Hide comment
@djberg96

djberg96 Apr 4, 2014

Hm, I ditched TCPSocket in the 1.7.0 release. Anyway, sorry for the confusion.

Hm, I ditched TCPSocket in the 1.7.0 release. Anyway, sorry for the confusion.

@ianheggie ianheggie deleted the ianheggie:multi_threaded_icmp_and_response_data branch Apr 7, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.