Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix icmp enetunreach bug #36

Closed
wants to merge 23 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

ianheggie commented Apr 2, 2014

This fixes the ICMP bug when pinging unreachable hosts - instead of throwing an exception it now returns false. The travis config has been updated to expect the bug to be fixed.

This branch is based on #35 extra_timeout_and_unreachable_tests pull request.

I hope you don't feel pressured with (yet) another pull request. Are you happy for me to submit further pull requests for other bugs fixes / enhancements or do you want me to wait till you have digested this and the previous pull request?

djberg96 and others added some commits Feb 26, 2014

@djberg96 djberg96 Commented out a test that usually failed. 3ccaee9
@ianheggie ianheggie Add travis tests for various ruby versions, with 1.8.7 and jruby mark…
…ed as allowed failures
e391c27
@ianheggie ianheggie Commented out jruby 18mode and 19mode till it is fixed so
we don't repeat three copies of it freezing for 10 minutes
1c0a6f7
@djberg96 djberg96 Merge pull request #29 from ianheggie/add_travis_tests
Add travis tests for various ruby versions, with 1.8.7 and jruby marked ...
55d00af
@ianheggie ianheggie Add ICMP tests to travis-ci (refactored exclusions to work
with rvmsudo);
Drop ruby 1.8.7 test;
Fix ping of 192.168.0.1 to use 127.0.0.1 instead;
Move handling of EXCLUDE_ICMP_TEST to the same place as other excludes;
9a0748f
@ianheggie ianheggie Add to Known Issues in README based on testing results 6c59c70
@djberg96 djberg96 Merge pull request #31 from ianheggie/add_icmp_tests_to_travis
Add icmp tests to travis, drop ruby 1.8.7, note known issues discovered in testing
c699a8a
@bcandrea bcandrea Fixed timeouts in Net::Ping::External
Using Timeout.timeout with Open3 is not an option with Ruby > 1.9.3
(see https://bugs.ruby-lang.org/issues/5487). Implemented a workaround
based on the code at https://gist.github.com/lpar/1032297
77fd65a
@bcandrea bcandrea Update travis.yml to trigger CI run
This update should trigger a Travis CI build on different Rubies.
6ae1b88
@djberg96 djberg96 Merge pull request #32 from bcandrea/develop
Fixed timeouts in Net::Ping::External
61cd0a4
@djberg96 djberg96 Don't use read_nonblock on Windows. bb83bc9
@bcandrea bcandrea Merge pull request #1 from djberg96/master
Merge back changes from djberg96
c034875
@bcandrea bcandrea Fix a typo in the Net::Ping::External test
This typo had the effect of setting the expected
ping duration to 5.5s instead of 1.5s. Since in Windows
the defaul ping timeout is 4s, the test always passed.
ef80ced
@bcandrea bcandrea Avoid implementing timeout on Windows
The -w option of ping on Windows has the same effect
of an external timeout.
9fe37ec
@bcandrea bcandrea Merge branch 'feature/fix-win32-external-ping' into develop c805658
@djberg96 djberg96 Merge pull request #33 from bcandrea/develop
Fix a typo in the unit test for Net::Ping::External and change the implementation for Windows
a4ae2bd
@djberg96 djberg96 Remove comment about win32-open3, add note about minimum Ruby version. 666474e
@ianheggie ianheggie Squashed 0944d8a (check_timeout) into one commit:
coSimplify and expand tests, adding tests for:
* timeout conditions
* unreachable hosts and routes
* open and unused ports

Added test/setup_ubuntu.bash to setup unreachable host, route,
and confirm we can still use telstra.com as a black hole
for pings and tests for ports 7, 22 and 1001

Refactored tests to line up with existing example in external

Add extra rvm versions;
Add IGNORE_HTTP_PING_DOES_NOT_WAITS_FOR_RESPONSE_BUG and
IGNORE_HTTP_PING_DOES_NOT_TIMEOUT_BUG to test those bugs with
failure allowed;

Reorganise .travis.yml to make intent clearer

Speed up timeout tests

Give more details on failing tests
9fdfc69
Owner

djberg96 commented on a4ae2bd Apr 1, 2014

Upon further reflection, I've decided to go back to just using the command line arguments. I think I lost sight of the purpose of the External class, which is to just wrap an external timeout. If folks need more granular timeout control, I think they shouldn't use an external ping to begin with.

I don't want you to think I don't appreciate your contribution - I do! And you will still get credit for spotting the useless Timeout wrapper and providing a solution.

ianheggie added some commits Apr 2, 2014

@ianheggie ianheggie Change UNREACHABLE_HOST to using a script to try and find an unreacha…
…ble host (using iptables doesn't work for host unreachable);

Don't enforce that icmp pings have to wait if they discover network unreachable early;
8453f54
@ianheggie ianheggie Merge remote-tracking branch 'upstream/master'
Conflicts: (resolved in favour of upstream changes):
	README
	lib/net/ping/external.rb
	test/test_net_ping_external.rb
f6bd725
@ianheggie ianheggie Merge branch 'master' (and upstream) into extra_timeout_and_unreachab…
…le_tests

Conflicts:
	test/test_net_ping_external.rb
a599135
@ianheggie ianheggie Fix ICMP ENETUNREACH Bug 8b4eb6f
@ianheggie ianheggie Remove TCP_UNREACHABLE_ROUTE_BUG flag and check if its still a problem d5e7514
Owner

djberg96 commented Apr 2, 2014

I'm sorry Ian, but I've decided to ditch Travis CI. First, without cross-platform support, it's not particularly useful to me. Second, I feel your testing approach (in conjunction with Travis CI) is not compatible with mine. I like to keep my tests simple and inline whenever possible. It's easier to deal with all around.

@djberg96 djberg96 closed this Apr 2, 2014

Contributor

ianheggie commented Apr 2, 2014

Ok ...

I am not sure what u mean by inline re tests. I am also curious what you
found complicated / conflicting and how you would have coded the tests to
locate the bugs I found.

Anyway I will change my fork to being published as it's own gem then as my
comfort level is for more test coverage and using a CI server ... I'll
probably call it net-ping2

Please feel free to cherry pick the fixes coming up in my fork and I will
do likewise.

All the best with your project.

Ian

Sent from my iPhone

On 2 Apr 2014, at 11:12 pm, Daniel Berger notifications@github.com wrote:

I'm sorry Ian, but I've decided to ditch Travis CI. First, without
cross-platform support, it's not particularly useful to me. Second, I feel
your testing approach (in conjunction with Travis CI) is not compatible
with mine. I like to keep my tests simple and inline whenever possible.
It's easier to deal with all around.

Reply to this email directly or view it on
GitHubhttps://github.com/djberg96/net-ping/pull/36#issuecomment-39322587
.

@ianheggie ianheggie deleted the ianheggie:fix_ICMP_ENETUNREACH_BUG branch Apr 4, 2014

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