Opened 10 years ago
Closed 7 years ago
#4330 enhancement closed fixed (fixed)
Allow the Deferred returned by Agent.request to be cancelled
Reported by: | Jean-Paul Calderone | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | web | Keywords: | |
Cc: | Erik Forsberg, Daniel Graña, Christian Kampka | Branch: |
branches/webclient-request-cancel-4330-5
branch-diff, diff-cov, branch-cov, buildbot |
Author: | itamarst, exarkun, habnabit |
Description (last modified by )
Cancelling the Deferred
returned by Agent.request
should fail the Deferred
with a CancelledError
and either:
- Cancel the connection setup attempt, if it is still in progress.
- Disconnect abort the in-progress request, if the connection has been set up and a request has been initiated.
This used to depend on #4329 to handle the former case; these days it depends on endpoints supporting cancellation.
Change History (33)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
Well, I basically finished #4329 already. And I like small changes.
comment:3 Changed 10 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/webclient-request-cancel-4330 |
(In [28848]) Branching to 'webclient-request-cancel-4330'
comment:4 Changed 9 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
comment:5 follow-up: 6 Changed 8 years ago by
A discussion on IRC brought up the point that Agent now uses endpoints instead of ClientCreator
, which are not yet cancelable.
comment:6 Changed 8 years ago by
Replying to jonathanj:
A discussion on IRC brought up the point that Agent now uses endpoints instead of
ClientCreator
, which are not yet cancelable.
What? Yes they are: http://twistedmatrix.com/documents/current/core/howto/endpoints.html#auto4
comment:8 Changed 8 years ago by
comment:9 Changed 8 years ago by
Author: | exarkun → exarkun, habnabit |
---|---|
Branch: | branches/webclient-request-cancel-4330 → branches/webclient-request-cancel-4330-2 |
(In [34323]) Branching to 'webclient-request-cancel-4330-2'
comment:10 Changed 8 years ago by
comment:11 Changed 7 years ago by
Keywords: | review added |
---|
comment:12 Changed 7 years ago by
Owner: | set to Itamar Turner-Trauring |
---|
comment:13 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Itamar Turner-Trauring to habnabit |
Thanks for implementing this! We should have more cancellable Deferreds. This is looking pretty good. However, some improvements are needed:
- Please write tests for cancelling after the response download has started, but hasn't finished. E.g. your download is too slow so you decide to stop it. To make sure you cover all cases maybe mention the covered state machine states in the cancellation tests, and make sure they match the full set of tests.
- Needs a news file.
- You should add a mention and/or example to the web client howto about the fact that you can cancel requests.
- If request was cancelled, and any bytes at all have been written (or for completeness probably always is easier), rather than using loseConnection, abortConnection should be used. Given that you may be dealing with uncooperative server, and/or you're giving up and don't care about writing any more, having the connection close immediately is what you want, as opposed to waiting until all write buffers are cleared, which may take an arbitrarily long amount of time.
comment:14 Changed 7 years ago by
habnabit pointed out, re point 1, that there is no Deferred at that point in the code. So I'm not sure what I was referring to.
comment:15 Changed 7 years ago by
Cc: | Erik Forsberg added |
---|
comment:16 Changed 7 years ago by
Cc: | Daniel Graña added |
---|
comment:17 Changed 7 years ago by
Branch: | branches/webclient-request-cancel-4330-2 → branches/webclient-request-cancel-4330-3 |
---|
(In [36263]) Branching to 'webclient-request-cancel-4330-3'
comment:19 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | habnabit deleted |
I've handle the review comments, build results here: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/webclient-request-cancel-4330-3
comment:20 Changed 7 years ago by
Cc: | Christian Kampka added |
---|
comment:21 Changed 7 years ago by
Owner: | set to Itamar Turner-Trauring |
---|---|
Status: | new → assigned |
comment:22 Changed 7 years ago by
Description: | modified (diff) |
---|---|
Keywords: | review removed |
Thanks for the update!
- Cancelling when in TRANSMITTING eventually results in
loseConnection()
; this should really beabortConnection()
. I believe the relevant code isebRequestWriting
inHTTP11ClientProtocol.request
. - For completeness sake it might be worth testing that cancellation works during the connection process, i.e. that
HTTPConnectionPool.getConnection
results are cancellable... but that just devolves to endpoint cancellation. So not sure quite what to do here. Probably nothing. - The auto-retry code in
_RetryingHTTP11ClientProtocol._shouldRetry
should be modified so it does not retry if the reason for failure was cancellation. - File a ticket for making the result of
FileBodyProducer.startProducing
cancellable.
I will address these comments and resubmit for review.
comment:23 Changed 7 years ago by
comment:24 Changed 7 years ago by
Author: | exarkun, habnabit → itamarst, exarkun, habnabit |
---|---|
Branch: | branches/webclient-request-cancel-4330-3 → branches/webclient-request-cancel-4330-4 |
(In [38500]) Branching to 'webclient-request-cancel-4330-4'
comment:25 Changed 7 years ago by
comment:26 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | Itamar Turner-Trauring deleted |
Status: | assigned → new |
- Done.
- Skipped.
- Done. Only tested one exception since it's the only one that can be the result of cancellation and cause a retry at the same time.
- Filed #6531.
Started a new buildbot run, but have not yet looked at results: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/webclient-request-cancel-4330-4
comment:27 Changed 7 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:28 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Itamar Turner-Trauring |
Status: | assigned → new |
Thanks! A few simple final comments:
- twisted/web/test/test_newclient.py
test_cancelDuringBodyProduction
callsHTTP11ClientProtocol.connectionLost
- several of the other new cancel tests don't, though. is there something special about this one?test_cancelDuringChunkedBodyProduction
seems pretty similar totest_cancelDuringBodyProduction
. Maybe the repetition could be factored out into a helper.- I agree that cancellation before connection setup is basically just endpoint cancellation. Maybe there should be a test for
HTTPConnectionPool.getConnection
that exercises this cancellation code path, though? Seems like it should be easy to write. If you get stuck, I wouldn't be opposed to seeing this done in a follow-up ticket.
- twisted/web/test/test_agent.py
- I see the test docstring follows the local convention of using "should", but maybe fixing the existing docstrings is simple enough that we can make it all follow the modern style instead?
test_onlyRetryIdempotentMethods
andtest_onlyRetryIfNoResponseReceived
should be split up into multiple tests. Can you file a ticket for that?
- twisted/web/client.py
- The change to
_shouldRetry
is incompletely tested. There is no test coverage for a_WrapperException
without aCancelledError
- The change to
That's all. Please address to your satisfaction and then merge. Thanks again to everyone who contributed!
comment:29 Changed 7 years ago by
- Done.
- Done, and #6533.
- Done.
Started another buildbot run, if all looks good will merge.
comment:30 Changed 7 years ago by
Owner: | changed from Itamar Turner-Trauring to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
Builds look good, I'll go ahead and merge this. Thanks everybody!
comment:31 Changed 7 years ago by
Branch: | branches/webclient-request-cancel-4330-4 → branches/webclient-request-cancel-4330-5 |
---|
(In [38596]) Branching to 'webclient-request-cancel-4330-5'
comment:33 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
#4329 may be a perfectly reasonable way to handle this, but would depending on #1442 instead be a reasonable option, or would that involve rewriting too much code?