Hi Manuel > The reason why I wrap all the errors in a HTTPRequestError is > because I wanted to have only one kind of error raised by a txsolr > request. This is handy in some cases, for example, we can catch > HTTPRequestError in the transaction module and retry the transaction > if anything wrong happens. This would't be possible if raise any > exception because then I don't know what exception to catch. For > instance, I don't know what kind of exceptions can be raised by the > twisted agent request, and that's not properly documented. I understand the need to catch HTTPRequestErrors and to retry etc. I just saying that you're turning *everything* into one of them and that's not good. For example, change the first line of to look like this: def responseCallback(response): if response.poopy blah (notice the missing colon on the if, the non-existent name). Then run your code and see what kind of error you get. You should be returning HTTPRequestError failure for HTTP errors and dealing with them via retry etc. Other errors can bubble through to higher levels, as is usual with Twisted, and in our case be logged and turned into 500 errors. Putting failure.trap(HTTPRequestError) into an errback that calls the client method is perfect. I mean there's already a very well understood Twisted mechanism for filtering out different kinds of errors. Your approach is like saying "I am infallible, nothing can possibly be wrong with my code, the only thing that could go wrong is an HTTP error" :-) > Maybe I can change the code to look like this: > > def responseErrback(failure): > result.errback(HTTPRequestError(failure.value)) That's putting an exception in an exception and still turning every error into an HTTPRequestError (I know that's what you *want*, but it doesn't correspond to programming reality - as we saw with self.errback, which was a real error that was being silently swallowed). How about: def responseCallback(response): _logger.debug('Received response from ' + url) if response.code == 200: deliveryProtocol = ResponseConsumer(result, JSONSolrResponse) response.deliverBody(deliveryProtocol) else: deliveryProtocol = DiscardingResponseConsumer() response.deliverBody(deliveryProtocol) raise HTTPWrongStatus(response.code) def responseErrback(failure): if failure.check(HTTPWrongStatus): result.errback(HTTPRequestError(failure.value)) else: # Whoah, unexpected error in my code! _logger.error(failure.value) result.errback(failure) d.addCallback(responseCallback) d.addErrback(responseErrback) return result It would be simpler to drop HTTPWrongStatus as it's not used anywhere as far as I can see. The above errback deals with both kinds of errors. If the thing that calls it attaches its own errback that does a failure.trap(HTTPRequestError) then any other error is more serious and will pass through (and presumably be caught by a higher level). > > [2] > > > > In response.py > > The code I wrote is correct. I want to decode the response even if I > don't get a ResponseDone reason. In fact, with our current Solr > setup, we never get a ResponseDone reason because the server is > using chunked transfer encoding and a Content-Length header is not > sent, thus we never get a ResponseDone reason. That's why I try to > decode the body first, and only if the procedure fails, I fire the > errback, otherwise I'll fire a callback with the decoded response. OK, got it, thanks. One comment is that if we're never getting ResponseDone then you're going to be writing an "Unclean response:" line to the log on every single Solr request, right? Is there any reason to do that if it's now the normal/expected behavior? How about: def connectionLost(self, reason): if reason.check(PotentialDataLoss, ResponseDone): try: response = self.responseClass(self.body) except Exception, e: _logger.error("Can't decode response body: %r" % self.body) self.deferred.errback(e) else: self.deferred.callback(response) else: _logger.warning('Unclean response: ' + repr(reason.value)) self.deferred.errback(reason) Or do you really want to process the body (and return it) on every single error? What does the JSONSolrResponse do if it's passed an empty string (self.body would presumably be empty for some possible errors). This also has the advantage that if there *is* some other kind of unexpected error, you're passing it on in the Twisted way. If Twisted happens to return a TheMoonExploded failure, you log it and pass it on. T