Skip to content
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

Can't handle exceptions on iOS in Net::Session#get #48

Open
jlmuir opened this issue Sep 26, 2016 · 7 comments
Open

Can't handle exceptions on iOS in Net::Session#get #48

jlmuir opened this issue Sep 26, 2016 · 7 comments

Comments

@jlmuir
Copy link
Contributor

jlmuir commented Sep 26, 2016

I have an iOS app where the user supplies a base URL for a service, and the app makes HTTP requests against the service via Flow's Net module. The app will crash with a RuntimeError while making an HTTP request if a problem occurs such as: the URL contains a host part that does not resolve in DNS, a connection to the service could not be established, etc. There seems to be no way to handle exceptions related to making the HTTP request.

To reproduce:

$ motion create Hello
$ cd Hello
$ echo "gem 'motion-flow'" >> Gemfile

Add the following (which contains a bad host part (i.e., a trailing x) in the base_url_supplied_by_user value) to AppDelegate#application:didFinishLaunchingWithOptions: after @window.makeKeyAndVisible:

base_url_supplied_by_user = 'https://httpbin.orgx'
session = Net.build(base_url_supplied_by_user) {}
begin
  session.get("/ip") do |response|
    if response.status == 200
      UI.alert({title: 'Client IP Address', message: response.body['origin'], default: 'OK'}) {}
    else
      UI.alert({title: 'HTTP Error', message: response.status_message, default: 'OK'}) {}
    end
  end
rescue RuntimeError => e
  UI.alert({title: 'Error', message: e.message, default: 'OK'}) {}
end

Then run it in a simulator:

$ rake

This results in a crash with the following message:

The operation couldn't be completed. (kCFErrorDomainCFNetwork error 310.) (RuntimeError)

Similarly, if no HTTP server is running on port 80 on the loopback device, changing the value of base_url_supplied_by_user to 'http://127.0.0.1' results in a crash with the following message:

Could not connect to the server. (RuntimeError)
@jjaffeux
Copy link
Contributor

jjaffeux commented May 4, 2017

We should probably wrap it inside Flow to offer the same error behavior on iOS/Android and to also simplify dev work.

Might have to change the API a little bit. Or add an "error" field on the response object.

session.get("/ip") do |response|
    if response.status == 200
      UI.alert({title: 'Client IP Address', message: response.body['origin'], default: 'OK'}) {}
    else
      p response.error # Net::Error
    end
  end

Net::Error would have some basic fields (message, code?) and also the classic Flow proxy field which would give access to the underlying platform error object.

@amirrajan what do you think?

@Bounga
Copy link

Bounga commented May 4, 2017

@jjaffeux Seems pretty good to me. I'm really for unifying the way it works across platform rather than having to rescue a specific exception. A response.error object would be enough for my usage.

@jjaffeux
Copy link
Contributor

jjaffeux commented May 4, 2017

@amirrajan @Bounga

A wild API idea:

# Multiple goals are achieved with this API
# - no nil check on response.error
# - no integer check on various status
# - more readable
# - clear separation between completion/failure of request
# and http success/error/... codes
session.get("/ip") do |response|
  response.complete { 
    on(:info) { } 
    on(:success) {
      UI.alert({title: 'Client IP Address', message: response.body['origin'], default: 'OK'}) {}
    }
    on(:redirect) { }
    on(:client_error) { }
    on(:server_error) { }
    on(:client_error, :server_error) { }
    on(300, 301, 304, 422, 500..503) { |error| some_logic(response.status) }
  }

  response.failure { |error| present_error_screen(error) }
end

@hboon
Copy link
Collaborator

hboon commented May 4, 2017

Interesting idea. Thanks for working on this. Been doing much Javascript lately? 😛

Some thoughts:

  1. This looks nice, but considering that Flow has a few other pieces, would a mix of paradigms be good?
  2. Does response.failure also cover 4xx and :client_error or just pure connectivity errors?
  3. on(:4xx) etc might be nice?

@amirrajan
Copy link
Collaborator

amirrajan commented May 4, 2017

I'm a big fan of Reqwest and HTTParty. Just my random two cents of projects I like.

@andrewhavens
Copy link
Collaborator

@jjaffeux I like these ideas. I think maybe it could be flattened a little bit to avoid too many nested blocks. What do you think about this:

request = session.get("/ip")
request.on(:success) do |response|
  UI.alert(title: 'Client IP Address', message: response.body['origin'], default: 'OK')
end
# I would assume that 3xx redirects would be performed automatically by default
request.on(:error) do |response|
  case response.status_code
  when 400..499
    UI.alert(title: 'You messed up', message: response.body['errors'], default: 'OK')
  when 500..599
    UI.alert(title: 'Server Error', message: response.message, default: 'OK')
  end
end
# or even simpler, as you mentioned:
request.on(:client_error) do |response|
  UI.alert(title: 'You messed up', message: response.body['errors'], default: 'OK')
end

...or maybe this:

session.get("/ip") do |response|
  response.on(:success) do
    UI.alert(title: 'Client IP Address', message: response.body['origin'], default: 'OK')
  end

  response.on(:client_error) do
    UI.alert(title: 'You messed up', message: response.body['errors'], default: 'OK')
  end
  # ...
end

...or maybe this:

session.get("/ip") do |response|
  if response.success?
    UI.alert(title: 'Client IP Address', message: response.body['origin'], default: 'OK')
  elsif response.client_error?
    UI.alert(title: 'You messed up', message: response.body['errors'], default: 'OK')
  elsif response.server_error?
    UI.alert(title: 'Server Error', message: response.message, default: 'OK')
  end
end

@jjaffeux
Copy link
Contributor

jjaffeux commented May 9, 2017

@andrewhavens @hboon thx for the great insights, I've been toying with multiple apis the last days, nothing finished yet.

My main concerns are:

  • differentiate https errors from network/uris/... errors
  • avoid multiple checking (success? error.nil? response.status.code == xxx...)
  • keep it very flexible and straightfoward

My personal reference is http://docs.python-requests.org/en/master/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants