Opened 5 years ago

Closed 19 months ago

#4330 enhancement closed fixed (fixed)

Allow the Deferred returned by Agent.request to be cancelled

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: web Keywords:
Cc: forsberg@…, dangra@…, christian@… Branch: branches/webclient-request-cancel-4330-5
(diff, github, buildbot, log)
Author: itamarst, exarkun, habnabit Launchpad Bug:

Description (last modified by itamar)

Cancelling the Deferred returned by Agent.request should fail the Deferred with a CancelledError and either:

  1. Cancel the connection setup attempt, if it is still in progress.
  2. 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 5 years ago by glyph

#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?

comment:2 Changed 5 years ago by exarkun

Well, I basically finished #4329 already. And I like small changes.

comment:3 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/webclient-request-cancel-4330

(In [28848]) Branching to 'webclient-request-cancel-4330'

comment:4 Changed 4 years ago by <automation>

  • Owner exarkun deleted

comment:5 follow-up: Changed 3 years ago by jonathanj

A discussion on IRC brought up the point that Agent now uses endpoints instead of ClientCreator, which are not yet cancelable.

comment:6 in reply to: ↑ 5 Changed 3 years ago by glyph

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:7 follow-up: Changed 3 years ago by exarkun

Unfortunately #5485

comment:8 in reply to: ↑ 7 Changed 3 years ago by glyph

Replying to exarkun:

Unfortunately #5485

Ouch. Well, thanks for the reference.

comment:9 Changed 3 years ago by habnabit

  • Author changed from exarkun to exarkun, habnabit
  • Branch changed from branches/webclient-request-cancel-4330 to branches/webclient-request-cancel-4330-2

(In [34323]) Branching to 'webclient-request-cancel-4330-2'

comment:10 Changed 3 years ago by habnabit

(In [34324]) Initial implementation (with tests!) of Agent request cancellation.

refs #4330

comment:11 Changed 2 years ago by habnabit

  • Keywords review added

comment:12 Changed 2 years ago by itamar

  • Owner set to itamar

comment:13 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner changed from itamar to habnabit

Thanks for implementing this! We should have more cancellable Deferreds. This is looking pretty good. However, some improvements are needed:

  1. 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.
  2. Needs a news file.
  3. You should add a mention and/or example to the web client howto about the fact that you can cancel requests.
  4. 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 2 years ago by itamar

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 2 years ago by forsberg

  • Cc forsberg@… added

comment:16 Changed 2 years ago by dangra

  • Cc dangra@… added

comment:17 Changed 2 years ago by habnabit

  • Branch changed from branches/webclient-request-cancel-4330-2 to branches/webclient-request-cancel-4330-3

(In [36263]) Branching to 'webclient-request-cancel-4330-3'

comment:18 Changed 2 years ago by habnabit

(In [36264]) Merging forward (refs #4330)

comment:19 Changed 20 months ago by therve

  • Keywords review added
  • Owner habnabit deleted

comment:20 Changed 20 months ago by chris-

  • Cc christian@… added

comment:21 Changed 19 months ago by itamar

  • Owner set to itamar
  • Status changed from new to assigned

comment:22 Changed 19 months ago by itamar

  • Description modified (diff)
  • Keywords review removed

Thanks for the update!

  1. Cancelling when in TRANSMITTING eventually results in loseConnection(); this should really be abortConnection(). I believe the relevant code is ebRequestWriting in HTTP11ClientProtocol.request.
  2. 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.
  3. The auto-retry code in _RetryingHTTP11ClientProtocol._shouldRetry should be modified so it does not retry if the reason for failure was cancellation.
  4. File a ticket for making the result of FileBodyProducer.startProducing cancellable.

I will address these comments and resubmit for review.

comment:23 Changed 19 months ago by itamarst

(In [38499]) If an error occurs when generating a request body (e.g. cancellation), close the connection with abortConnection() rather than loseConnection().

Refs #4330

comment:24 Changed 19 months ago by itamarst

  • Author changed from exarkun, habnabit to itamarst, exarkun, habnabit
  • Branch changed from branches/webclient-request-cancel-4330-3 to branches/webclient-request-cancel-4330-4

(In [38500]) Branching to 'webclient-request-cancel-4330-4'

comment:25 Changed 19 months ago by itamarst

(In [38502]) Don't retry requests that failed due to user cancellation.

Refs #4330

comment:26 Changed 19 months ago by itamar

  • Keywords review added
  • Owner itamar deleted
  • Status changed from assigned to new
  1. Done.
  2. Skipped.
  3. 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.
  4. 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 19 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:28 Changed 19 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar
  • Status changed from assigned to new

Thanks! A few simple final comments:

  1. twisted/web/test/test_newclient.py
    1. test_cancelDuringBodyProduction calls HTTP11ClientProtocol.connectionLost - several of the other new cancel tests don't, though. is there something special about this one?
    2. test_cancelDuringChunkedBodyProduction seems pretty similar to test_cancelDuringBodyProduction. Maybe the repetition could be factored out into a helper.
    3. 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.
  2. twisted/web/test/test_agent.py
    1. 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?
    2. test_onlyRetryIdempotentMethods and test_onlyRetryIfNoResponseReceived should be split up into multiple tests. Can you file a ticket for that?
  3. twisted/web/client.py
    1. The change to _shouldRetry is incompletely tested. There is no test coverage for a _WrapperException without a CancelledError

That's all. Please address to your satisfaction and then merge. Thanks again to everyone who contributed!

comment:29 Changed 19 months ago by itamar

  1. Done.
  2. Done, and #6533.
  3. Done.

Started another buildbot run, if all looks good will merge.

comment:30 Changed 19 months ago by exarkun

  • Owner changed from itamar to exarkun
  • Status changed from new to assigned

Builds look good, I'll go ahead and merge this. Thanks everybody!

comment:31 Changed 19 months ago by exarkun

  • Branch changed from branches/webclient-request-cancel-4330-4 to branches/webclient-request-cancel-4330-5

(In [38596]) Branching to 'webclient-request-cancel-4330-5'

comment:32 Changed 19 months ago by exarkun

(In [38598]) put the news fragment in the right place

refs #4330

comment:33 Changed 19 months ago by exarkun

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [38599]) Merge webclient-request-cancel-4330-5

Author: habnabit, therve, itamar
Reviewer: itamar, exarkun
Fixes: #4330

Implement resource cleanup for cancellation of the Deferred returned by
twisted.web.client.Agent.request.

Note: See TracTickets for help on using tickets.