Opened 13 years ago

Closed 12 years ago

#3811 enhancement closed fixed (fixed)

Allow aborting HTTP client connections in the _newclient API

Reported by: radix Owned by:
Priority: normal Milestone:
Component: web Keywords: httpclient
Cc: Branch: branches/expressive-client-timeouts-3811
branch-diff, diff-cov, branch-cov, buildbot
Author: radix


It should be possible to abort connections that are in progress in the _newclient API. This should close the connection and notify all application-callback APIs that their requests have been aborted.

Change History (13)

comment:1 Changed 13 years ago by radix

Keywords: review added

This is ready for review in the bzr branch at

This bzr branch is based on the branch expressive-http-client-886-3, which is accessible via bzr at:

If another format would be preferable to a reviewer, please feel free to request it.

comment:2 Changed 13 years ago by radix

Owner: jknight deleted

comment:3 Changed 13 years ago by ivank

In twisted/web/test/, the name deliverBodyAndReturnResponse is not defined. The tests don't pass.

comment:4 Changed 13 years ago by ivank

  1. There should be vertical space after the end of def deliverBody(response) .
  1. from twisted.web._newclient import ChunkedEncoder, RequestGenerationFailed, RequestTransmissionFailed, ResponseFailed, WrongBodyLength, RequestNotSent, ConnectionAborted is 169 characters. Perhaps it should be no longer than the second-longest import in

comment:5 Changed 13 years ago by ivank

Keywords: review removed

I can't review the necessity of the abort, so someone else should do that.

comment:6 Changed 13 years ago by radix

Keywords: review added
  1. The NameError has been fixed.
  2. Fixed.
  3. I've resized this line to 80 columns. I didn't clean up *all* the >80 column lines in this branch to prevent unnecessary conflicts against the branch that this is based off of, since it hasn't yet been merged to trunk.

Thanks for the review.

comment:7 Changed 13 years ago by Glyph

Keywords: review removed
Owner: set to radix

It looks... okay, I guess? I can't tell how to get a diff out of the branch. Also, what exactly is the protocol for reviewing a branch based on another branch? I guess if ... #886 lands without modification, then...

No, I don't know what to do. This is outside the usual flow of things in too many different ways for me to deal with at once. What I think are the changes in this branch appear be fine to be landed on trunk, if trunk were #886 already. But I can't tell buildbot to do anything with it.

Unfortunately I think if I saw some serious problem with the code itself this review would be much easier. Please make your expectations for the output of this review, and some directions for how the reviewer should proceed, a little clearer.

comment:8 Changed 13 years ago by radix

Okay, so here's how you'd review this change:

cd whatever
bzr branch
cd expressive-http-client-886-3
bzr merge
bzr diff | less
bzr revert

There are two things we could do with a +1: merge this branch into the expressive-http-client-886-3 timeout, or just leave it as a branch until expressive-http-client-886-3 is merged, and then merge this branch immediately after. Both of these options have some problems. The first one makes reviewing the final branch more confusing, or at least harder. The second one means maintaining divergences and resolving conflicts. I think we should do the second one, because hopefully we can get expressive-http-client merged to trunk quickly.

So I'm happy with a "+1, don't merge this branch".

comment:9 Changed 12 years ago by Jean-Paul Calderone

Keywords: httpclient review added
Owner: radix deleted

#866 and #3987 are in trunk now. That should make the review process for this ticket pretty straightforward.

comment:10 Changed 12 years ago by radix

Branch: branches/expressive-client-timeouts-3811

(In [27665]) Branching to 'expressive-client-timeouts-3811'

comment:11 Changed 12 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to radix

This seems to be good, except it's missing a news topfile. Add that, and if the build succeeds on all platforms then please merge.

comment:12 Changed 12 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [28528]) Merge expressive-client-timeouts-3811

Author: radix Reviewer: ivank, glyph, exarkun Fixes: #3811

Add HTTP11ClientProtocol.abort to interrupt a request/response early by dropping the connection.

comment:13 Changed 11 years ago by <automation>

Owner: radix deleted
Note: See TracTickets for help on using tickets.