Ticket #4330 enhancement new

Opened 3 years ago

Last modified 3 weeks ago

Allow the Deferred returned by Agent.request to be cancelled

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: web Keywords: review
Cc: forsberg@…, dangra@…, christian@… Branch: branches/webclient-request-cancel-4330-3
Author: exarkun, habnabit Launchpad Bug:

Description

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 depends on #4329 to handle the former case.

Change History

1

  Changed 3 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?

2

  Changed 3 years ago by exarkun

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

3

  Changed 3 years ago by exarkun

  • branch set to branches/webclient-request-cancel-4330
  • branch_author set to exarkun

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

4

  Changed 2 years ago by <automation>

  • owner exarkun deleted

5

follow-up: ↓ 6   Changed 15 months ago by jonathanj

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

6

in reply to: ↑ 5   Changed 15 months 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

7

follow-up: ↓ 8   Changed 15 months ago by exarkun

Unfortunately #5485

8

in reply to: ↑ 7   Changed 15 months ago by glyph

Replying to exarkun:

Unfortunately #5485

Ouch. Well, thanks for the reference.

9

  Changed 13 months ago by habnabit

  • branch changed from branches/webclient-request-cancel-4330 to branches/webclient-request-cancel-4330-2
  • branch_author changed from exarkun to exarkun, habnabit

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

10

  Changed 13 months ago by habnabit

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

refs #4330

11

  Changed 10 months ago by habnabit

  • keywords review added

12

  Changed 10 months ago by itamar

  • owner set to itamar

13

  Changed 10 months 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.

14

  Changed 9 months 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.

15

  Changed 9 months ago by forsberg

  • cc forsberg@… added

16

  Changed 8 months ago by dangra

  • cc dangra@… added

17

  Changed 7 months 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'

18

  Changed 7 months ago by habnabit

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

19

  Changed 5 weeks ago by therve

  • keywords review added
  • owner habnabit deleted

20

  Changed 3 weeks ago by chris-

  • cc christian@… added
Note: See TracTickets for help on using tickets.