Implement fail_for_status

[dmurvihill]

This commit adds a fail_for_status method to the Response object analogous to the 'raise_for_status' method in Request, to save users some time writing boilerplate status code checking.

[codecov-io]

Codecov Report

Merging #159 into master will decrease coverage by 2.01%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage   98.71%   96.69%   -2.02%     
==========================================
  Files          20       20              
  Lines        1713     1817     +104     
  Branches      136      155      +19     
==========================================
+ Hits         1691     1757      +66     
- Misses         12       39      +27     
- Partials       10       21      +11

Impacted Files	Coverage Δ
src/treq/response.py	92.85% <100%> (-3.92%)	:arrow_down:
src/treq/__init__.py	100% <100%> (ø)	:arrow_up:
src/treq/test/test_response.py	89.74% <100%> (-10.26%)	:arrow_down:
src/treq/test/test_testing.py	100% <100%> (ø)	:arrow_up:
src/treq/test/util.py	53.12% <0%> (-46.88%)	:arrow_down:
src/treq/test/test_multipart.py	92.82% <0%> (-4.89%)	:arrow_down:
src/treq/test/test_treq_integration.py	94.04% <0%> (-0.93%)	:arrow_down:
src/treq/client.py	97.58% <0%> (-0.39%)	:arrow_down:
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 22937cd...4b51d3c. Read the comment docs.

[dmurvihill]

Hi @exarkun, I pushed an update. I believe the only question outstanding is whether you want to use the status code constants from Twisted even though integer literals are used in the rest of the test file.

[glyph]

It looks like this branch might have gotten mixed up with #157 ?

[dmurvihill]

Oh boy... I'll fix that later today.

[dmurvihill]

k @glyph @exarkun, this is fixed.

[Julian]

I think the implementation here should match the existing ones -- a global function that you can attach as a callback, and then a method that calls it on self if you already have a Response object. The convenient pattern seems like it's been treq.get("http://example.com").addCallback(treq.content), so it'd be nice to be able to inject this into that -- i.e. treq.get("http://example.com").addCallback(treq.fail_for_status).addCallback(treq.content)

[dmurvihill]

Thanks @Julian, the method check_status has been made available as a global function.

[dmurvihill]

Fixed conflicts and squashed the commits. Is anyone interested in this feature?

[glyph]

This is definitely something Treq should have in some capacity.

I can understand if @dmurvihill isn't still around to finish responding to review and clean up conflicts - if not, @twm, do you think you could take this over and drive it to landing?

[twm]

@glyph Sure, though it may be a while (a few weeks) as I'm pretty busy right now.

[graingert]

why return a failure rather than raise an exception?

[Julian]

Failure is the asynchronous version of an exception -- the error has to be delivered to whoever made the request, not in the middle of the reactor spinning itself.

(/me still +1 too for this being great to add)

[graingert]

@Julian but constructing the failure is synchronous, it's misleading to return a deferred unless you expect to perform asynchronous work

async def amain():
    response = await treq.get(url)
    response.raise_for_status()

defer.ensureDeferred(amain())

[graingert]

Failure is the asynchronous version of an exception -- the error has to be delivered to whoever made the request, not in the middle of the reactor spinning itself.

(/me still +1 too for this being great to add)

What makes you think the error would be delivered to anyone other than the caller of raise_for_status?

[twm]

"Fail" is the right term because the API needs to consume the response body. Something like:

@defer.inlineCallbacks
def txmain(reactor):
    response = yield treq.get(url)
    yield response.expect_status()
    body = yield response.content()

task.react(txmain, ())

It's important to consume the body because otherwise the connection won't be returned to the pool (effectively it leaks until the server times it out — 🤞 the server will do that before you run out of FDs).

This is a bit awkward and I think it would be prone to misuse — it's not obvious why you'd need to yield such a function, so folks are likely to forget to do so. For that reason I was considering making it part of the initial call. Something like:

@defer.inlineCallbacks
def txmain(reactor):
    response = yield treq.get(url, expect_status=200)
    body = yield response.content()

This is inspired by Elm's HTTP package.

We might want to have a more general "expectation" concept, which could handle things like timeouts, maximum response size, response Content-Type, etc. Treq seems like a good place for this stuff as it's the "higher level" HTTP library.

[graingert]

But why would fail/raise_for_status consume the body? Avoiding resource leaks is a separate concern:

async def amain():
    async with treq.get(url) as response:
        response.raise_for_status()
        body = await response.content()

task.react(lambda _: defere.ensureDeferred(amain(), ())

[twm]

Ah, I still live in a Python 2.7-compatible world, so async with didn't occur to me. However the other thing I had in mind is including the response body (or size-limited version thereof) in the exception.

[graingert]

You can still avoid resource leaks in py2, using a continuation like _ConcurrencyMixin:

@defer.inlineCallbacks
def txmain():
    @defer.inlineCallbacks
    def continuation(response)
        response.raise_for_status()
        body = yield response.content()

    yield treq.get(url)(continuation)

task.react(lambda _: txmain(), ())

I don't see a need for raise_for_status to consume the body and/or return a Deferred, what if the response is an EventSource and never completes?

[twm]

I don't follow your example. How does it consume the body when raise_for_status() raises an exception?

As for a streaming response, that's what timeouts and response size limits are for.

[graingert]

I don't follow your example. How does it consume the body when raise_for_status() raises an exception?

the continuation here is like the body of a context manager, when the continuation ends treq can close the channel associated with the request: (There's no need to consume the body)

treq.get can return an instance of:

class _ResponseDeferred(defer.Deferred):
    @defer.inlineCallbacks
    def __call__(self, continuation):
        response = yield self
        try:
            defer.returnValue((yield continuation(response)))
        finally:
            yield closeChannel(response)

[glyph]

No activity since 2019, it seems that there are still some open design questions, and there are conflicts. Feel free to resurrect and reopen but I'm going to close to remove clutter from the PR list.