Allow aborting HTTP client connections in the _newclient API

[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.

[radix]

This is ready for review in the bzr branch at

​http://twistedmatrix.com/users/radix/twisted-branches/expressive-client-timeouts/

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

​http://svn.twistedmatrix.com/bzr/Twisted/branches/expressive-http-client-886-3/

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

[ivank]

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

[ivank]

2. There should be vertical space after the end of def deliverBody(response) .

3. 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 test_newclient.py

[ivank]

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

[radix]

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.

[glyph]

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.

[radix]

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

cd whatever
bzr branch http://svn.twistedmatrix.com/bzr/Twisted/branches/expressive-http-client-886-3/
cd expressive-http-client-886-3
bzr merge http://twistedmatrix.com/users/radix/twisted-branches/expressive-client-timeouts/
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".

[exarkun]

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

[radix]

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

[exarkun]

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.

[exarkun]

(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.