Allow Features to handle connection errors

[joshuaflanagan]

This addresses #547

It allows a Feature to implement an on_error handler so that it has a chance to react to Requests that never get a Response.

[tarcieri]

Looks like you need to fix the RuboCop errors. Otherwise conceptually this seems ok to me.

@ixti what do you think?

[tarcieri]

@paul any thoughts on this approach, or suggested alternatives?

[paul]

@tarcieri No, this seems fine to me. 👍

[tarcieri]

I think you can do each_value here (and potentially collapse it into a 1-liner), e.g.:

options.features.each_value { |feature| feature.on_error(req, e) }

[joshuaflanagan]

Looks like you need to fix the RuboCop errors. Otherwise conceptually this seems ok to me.

It is difficult as an outside contributor to understand which rubocop rules a team cares about. Were each of the rules chosen because they are what the team values? Or were they just the defaults?

For example, this project currently has the Metrics/MethodLength rule enabled, so this PR has this violation:

lib/http/client.rb:66:5: C: Metrics/MethodLength: Method has too many lines. [32/25]
    def perform(req, options)

Is it really appropriate for this PR to take on the task of refactoring the Client#perform method so that it isn't so long?

Or should I disable the rule? Or just exclude this file? Or just exclude this method?

[tarcieri]

It is difficult as an outside contributor to understand which rubocop rules a team cares about. Were each of the rules chosen because they are what the team values? Or were they just the defaults?

It's explicitly configured to 25 lines in .rubocop.yml. To answer your question: yes this project values smaller methods.

Is it really appropriate for this PR to take on the task of refactoring the Client#perform method so that it isn't so long?

If you really don't want to refactor it you can disable the check for this method in particular using rubocop:disable and leave it for someone else to refactor. At the very least the RuboCop check needs to pass.

That said it looks like Response.new could benefit from some refactoring and is the source of much of the methods bulk by lines.

[joshuaflanagan]

If you really don't want to refactor it you can disable the check for this method in particular using rubocop:disable and leave it for someone else to refactor. At the very least the RuboCop check needs to pass.

I don't mind doing the refactor, I just thought it was out of scope for this task, and would obscure the point of this PR. I'll go ahead and disable the rule here, and leave it as a follow up issue.

[ixti]

What if error will happen down the course, e.g. during response streaming?

[tarcieri]

@ixti it seems like under this approach features wouldn't be able to handle those errors, however it seems like they can handle the most interesting errors, i.e. immediate response errors

[joshuaflanagan]

I have rebased and addressed the rubocop issues.

[ixti]

Thank you!

I have taken another look, and I think we can make API a bit more "flexible" if we will rename on_error to observe:

def observe(event, data); end

# ...

options.features.each_value { |f| f.observe(:request_error, :req => self, :err => e) }

Just an idea. As I don't like on_error name it's too vague, while used in very specific case only. Also, probably we should add observers in general instead of bloating this functionality into features.

[tarcieri]

@ixti what about on_event that takes a symbol (e.g. :error) as an argument?

[joshuaflanagan]

@ixti what about on_event that takes a symbol (e.g. :error) as an argument?

I think that could be a good idea if you know of other events that would make sense to observe.
I could imagine having :request and :response events, which could replace the need to "wrap" those objects for all use cases. I understand the wrapping approach is required for some features, but it leads to an awkward implementation for things like logging, which don't need to modify the request/response.

[ixti]

Yeah. I think on_event makes sense to me.

And I agree, that :request and :response events will make notifications (which are actually observers) code look much better. Also after giving it another look, I guess I'm not very against on_error name any more, we can create a bunch of on_{event} methods:

def on_request(req:); end
def on_request_error(req:, err:); end
def on_response(res:); end
def on_response_error(res:, err:); end

So that it will be absolutely obvious which kwargs will be passed.

[joshuaflanagan]

Can we move forward with this PR and leave the on_request/on_response changes for a follow-up effort?
I'm not exactly sure how you want to differentiate between on_request_error and on_response_error, but willing to make the changes with some direction.

[tarcieri]

@joshuaflanagan that sounds fine to me

[joshuaflanagan]

Done

[tarcieri]

@joshuaflanagan this one now has a conflict

[tarcieri]

@ixti let me know if you have any qualms about landing this. We can potentially explore alternatives before cutting another release