godfat / rest-core

Various rest-builder middleware for building REST clients.
Apache License 2.0
57 stars 13 forks source link

Connection refused does not emit exception, future just returns nil instead #6

Closed orospakr closed 9 years ago

orospakr commented 10 years ago
require "rest-core"
SillyClient = RC::Builder.client do
  use RC::JsonResponse, true
  # localhost is not listening on 3141, so the IP stack just returns RST
  # instantly
  use RC::DefaultSite, "http://localhost:3141/"
  use RC::CommonLogger, method(:puts)
end

myclient = SillyClient.new
myclient.get("whatever").to_s # -> gives me nil, no exception thrown.

Thank you!

godfat commented 10 years ago

I guess this is not a good design, and it is quite confusing, but if you also use RC::ErrorHandler somewhere in the stack, even without any real error handler, it would raise an exception. Full example:

require "rest-core"
SillyClient = RC::Builder.client do
  use RC::ErrorHandler, nil # Even if error handler is simply a nil...
  use RC::JsonResponse, true
  # localhost is not listening on 3141, so the IP stack just returns RST
  # instantly
  use RC::DefaultSite, "http://localhost:3141/"
  use RC::CommonLogger, method(:puts)
end

myclient = SillyClient.new
myclient.get("whatever").to_s # -> raises Errno::ECONNREFUSED

This is because the error would be recorded in env[RC::FAIL], and only RC::ErrorHandler would be looking into it, and even if there's no handler at all, it would still raise it.

What do you think we could improve this? Still raise an exception even if no RC::ErrorHandler presents? In this case, what should we expect RC::ErrorHandler to do?

orospakr commented 10 years ago

Hm, OK, I was confused because HTTP level errors (4xx, etc.) were emitting exceptions, iirc. Is that the currently expected behaviour?

godfat commented 10 years ago

Not really. Here's an example which would not raise any exceptions for 404. You probably got the exception from RC::JsonResponse since it cannot parse a 404 page? I agree this is inconsistent and very confusing though :/ I don't really expect this, but this is how it is working right now.

require "rest-core"
SillyClient = RC::Builder.client do
  use RC::DefaultSite, "http://www.google.com/"
  use RC::CommonLogger, method(:puts)
end

myclient = SillyClient.new
myclient.get("whatever").to_s # -> gives me nil, no exception thrown.

I should probably fix this so that it is consistent, for example, no exception would ever be raised if RC::ErrorHandler was not used, or it would always raise some exceptions regardless the presence of RC::ErrorHandler.

godfat commented 9 years ago

Now exceptions are raised in RC::Client instead of RC::ErrorHandler, so it would always raise an exception now!

All RC::ErrorHandler should be doing should be creating the error object instead of raising it. It's separation of concerns or single responsibility principle.