HipByte / Flow

Cross-platform libraries for RubyMotion
BSD 2-Clause "Simplified" License
141 stars 29 forks source link

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

Open jlmuir opened 8 years ago

jlmuir commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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

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 commented 7 years ago

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

andrewhavens commented 7 years ago

@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 commented 7 years ago

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

My main concerns are:

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