Code review comment for lp://staging/~ceronman/txsolr/response-consumer-errback-843429

Hi Manuel

[1]

In client.py you have

         def responseErrback(failure):
             result.errback(HTTPRequestError(failure))

I'm not sure why you do it that way. Why not just pass the failure to
the errback?

Something went wrong, and you already have a failure, so pass it along.

If you put the failure into an HTTPRequestError instance and pass that
to the errback, the errback will wrap the exception instance in a
failure, which would give a failure containing a exception containing
a failure containing an exception.

By unconditionally wrapping every error into an HTTPRequestError
you're silently turning things that are not HTTP errors (like Python
syntax errors or the self.errback AttributeError - i.e., things that
might be/go wrong in responseCallback) into HTTPRequestErrors.

[2]

In response.py, I think this should be rewritten:

    def connectionLost(self, reason):
        if not reason.check(ResponseDone):
            _logger.warning('Unclean response: ' + repr(reason.value))

        try:
            response = self.responseClass(self.body)
        except SolrResponseError, e:
            _logger.error("Can't decode response body: %r" % self.body)
            self.deferred.errback(e)
        else:
            self.deferred.callback(response)

to

    def connectionLost(self, reason):
        if reason.check(ResponseDone):
            try:
                response = self.responseClass(self.body)
            except SolrResponseError, e: # better: use Exception
                _logger.error("Can't decode response body: %r (%s)" % (
                    self.body, e))
                self.deferred.errback(e)
            else:
                self.deferred.callback(response)
        else:
            _logger.warning('Unclean response: ' + repr(reason.value))
            self.deferred.errback(reason)

The code as it stands will try to decode the response when you get
something other than ResponseDone, and you only want to do that when
the connection is closed properly.

Question: Are you *sure* that the only possible exception from calling
self.responseClass(self.body) is a SolrResponseError? I don't find
that code very reassuring - what happens if some other exception
occurs? I think it would be better to use Exception to catch other
possible exceptions at that point. Anything that goes wrong, you give
it to the errback.

I also added logging of the exception when the response decoding fails
(I mean printing the exception so we can see what happened). Ideally
that kind of error would be translated to an Internal Server Error if
Fluidinfo ran into it.