not possible to handle connection errors

[phulst]

If the service that a weary client is calling is not accessible at all, the perform method does not call the block that was passed in. This is understandable to some degree, as you may not be able to construct a Weary::Response object without a body, status or headers. But lacking some kind of error callback it is tricky to determine whether the call was successfully made.

In my app, I need to track whether the weary call failed, so some type of error callback would be very helpful. I'd be happy to write this code. Let me know if you'd be willing to accept a pull request that adds this functionality and if you have preferences on how this should be implemented.

[phulst]

  future do
    status, headers, body = call(rack_env_defaults)
    response = Weary::Response.new body, status, headers
    yield response if block_given?
    response
  end

the call() method will throw an exception if the web service cannot be reached, causing the yield to never happen.

[mwunsch]

Great issue to bring up. I'd like to chat a little bit about how Weary should behave in this scenario.

The first approach that comes to mind is that we rescue any exception that comes from Request#call by returning a 502: Bad Gateway response, perhaps with the error message in the body. This feels semantically correct. In the case of using Weary as a Rack application it becomes transparent, and in the case of using Weary simply as a web service client, it's easy to detect and recover from.

I don't think that we should introduce another callback mechanism beyond the block passed to #perform eg. an #on_error method or something like that.

It may even be worthwhile to subclass Weary::Response and introduce a Weary::FaultyRequest to be able to get more information about the cause of the exception.

What did you have in mind?

[phulst]

see above commit for what I'm thinking.

I agree that using the same callback and Response object is probably the best approach, even though strictly speaking in case of a network error you're not dealing with a response, but rather with a lack of a response.

In regards to what to set in that Response object: The problem with using existing status codes (like 502) is that it will then be more difficult to distinguish a 'not able to connect' error with an actual 502 error that the service itself may return. Some users of your library may already have special handling for a 502 status code and this new error handling could mess with that. So if we had to set a status in your Response object, it may be best to set a value that cannot be confused with any status code that could be returned by the service itself, that's why I'm just setting it to 0. I haven't tested the above commit yet but let me know what you think. I added a connection_error property to the Response object that is set, so the developer can inspect this to get more info about the error.

[phulst]

i tested my patch, fixed a minor thing and sent you a pull request. This request also reverts the rake requirement to 1.4.5 or higher instead of ~> 1.5

[mwunsch]

I closed #35. These two concerns are separate. Let's have the Rack issue be a separate pull request (or I can address it quickly in a commit).

Your approach in having an attr_accessor also doesn't sit right with me. I think Weary::Response should be immutable. I also think it might be more worthwhile to subclass Weary::Response in the case where there's an exception.

Instead of a status code of 0, we should consider using a code in the 5\ range (See: http://tools.ietf.org/html/rfc2616#section-6.1.1). I think it ought to use as much of the standard HTTP/Rack machinery as possible.

It might be easier if you explain your use case? What would you do in the callback when an exception occured that you wouldn't do by detecting a 502 or 5\ error, or the class of the response itself?

[phulst]

ok. close my updated pull request for now - I sent that before I saw your response here.

My issue with using a 5xx status code was that they all represent a response from the (or a) server. In the same way, subclassing Response indicates that we're dealing with a type of response, and it felt like 'no response' isn't a type of response. But thinking about it more, I think you can make the argument that it's still a response, but just from a different point on the network. The only reason why I was hesitant to set a 5xx code is because certain users may already have specific error handling code dealing with that specific status code. They may or may not want to handle this type of connection error differently, I don't know. In my specific use case I handle all non-200 responses the same way so for me personally using a 5xx status would work fine.

I don't feel strongly about subclassing the Response though I don't see a huge benefit to it either. A different type is always nice but if you simply subclass Response it ends up with all kinds of properties that don't apply on this error type. It would almost make more sense to have Weary::Response be the subclass then, but that feels like a hack too.

[mwunsch]

But thinking about it more, I think you can make the argument that it's still a response, but just from a different point on the network.

I think that's what I'm getting at. Weary is a kind of web server, and so an error in it should manifest itself as just another error in the pipeline. I'll try to hack something together...

[phulst]

I made the error immutable and now setting the status to 502, but did not subclass Response. Will send you a pull request. You can accept it as-is, add the subclassing to it, or roll your own solution entirely... your call. Thanks for weighing in on all this.