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

DefaultInternetObservingStrategy Does not Correctly Observe "Internet Walled Garden" State When it Should #116

Closed
epicstar opened this Issue Oct 25, 2016 · 2 comments

Comments

2 participants
@epicstar

epicstar commented Oct 25, 2016

DefaultInternetObservingStrategy currently doesn't account for the case that:

  1. cell phone data is off
  2. wifi is connected to a hotspot that "asks you to sign in" (airport wifi or coffee shops, etc.)

Steps to Reproduce

  1. Turn off data on phone
  2. Go to a place that has a "walled garden internet". For example..
    a. go to Starbucks
    b. Connect to their wifi but don't sign in
    c. ensure that your phone/device is asking you to "sign in" into their network
  3. Subscribe to ReactiveNetwork.observeInternetConnectivity()

Expected

The observable should emit "False" since there is no internet available until you "sign into" the network

Actual

The observable is returning true.

Thoughts

Apparently POSIX's ping (ping fails) works as expected and I was expecting the same behavior with the Socket class in Java, but that's not the case at all.

I looked into the issue via SO and found this which works: http://stackoverflow.com/questions/10813635/checking-internet-connection-on-wifi-hotspot

Good news is that you designed the ReactiveNetwork API pretty well so I simply had to just create my own class that implemented the InternetStrategy interface, skim through your source code, and wrapped the solution above in the class:

https://github.com/rectangle-dbmi/Realtime-Port-Authority/blob/default/app/src/main/java/rectangledbmi/com/pittsburghrealtimetracker/utils/WalledInternetStrategy.java

This internet strategy not only listens to internet status but also to the walled garden case. The only problem with this code right now is it won't work in China because of their amazing "Great Firewall" which makes the google endpoint I'm creating requests for return a 302 instead of a 204.

Two things:

  • I wouldn't mind creating a PR for this new Internet Strategy but I'll have to create tests for it first
  • I think this is an important bug to fix and perhaps my current code perhaps warrants another design discussion for listening to internet connectivity

@pwittchen pwittchen added the bug label Oct 25, 2016

@pwittchen

This comment has been minimized.

Show comment
Hide comment
@pwittchen

pwittchen Oct 25, 2016

Owner

Thank you for reporting this issue with such detailed description. Internet Walled Garden is an interesting use case and I haven't taken it into consideration.

PR with improvements is welcome of course.

You can also add a few unit tests as you mentioned. ReactiveNetwork now uses Robolectric for unit testing, so creating new tests would be more convenient than before. You can also find exemplary unit tests in this repository.

Probably we also need to rename SocketErrorHandler and its method to something more general because your new strategy is not using a socket, but it could have error handling functionality. I've added separate issue for that in #118.

Regards,
Piotr

Owner

pwittchen commented Oct 25, 2016

Thank you for reporting this issue with such detailed description. Internet Walled Garden is an interesting use case and I haven't taken it into consideration.

PR with improvements is welcome of course.

You can also add a few unit tests as you mentioned. ReactiveNetwork now uses Robolectric for unit testing, so creating new tests would be more convenient than before. You can also find exemplary unit tests in this repository.

Probably we also need to rename SocketErrorHandler and its method to something more general because your new strategy is not using a socket, but it could have error handling functionality. I've added separate issue for that in #118.

Regards,
Piotr

@pwittchen pwittchen modified the milestone: Fixing bugs & making enhancements Mar 7, 2017

@pwittchen pwittchen self-assigned this Mar 20, 2017

@pwittchen pwittchen closed this in #196 Aug 1, 2017

@pwittchen

This comment has been minimized.

Show comment
Hide comment
@pwittchen

pwittchen Aug 1, 2017

Owner

Bug fixed and will be available in the next release both RxJava1.x and RxJava2.x version.

PS. Better late than never ;-).

Owner

pwittchen commented Aug 1, 2017

Bug fixed and will be available in the next release both RxJava1.x and RxJava2.x version.

PS. Better late than never ;-).

@pwittchen pwittchen referenced this issue Aug 1, 2017

Closed

Release 0.11.0 #194

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