Opened 10 years ago
Closed 9 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 |
Description
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 10 years ago by
Keywords: | review added |
---|
comment:2 Changed 10 years ago by
Owner: | jknight deleted |
---|
comment:3 Changed 10 years ago by
In twisted/web/test/test_newclient.py, the name deliverBodyAndReturnResponse
is not defined. The tests don't pass.
comment:4 Changed 10 years ago by
- There should be vertical space after the end of
def deliverBody(response)
.
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
comment:5 Changed 10 years ago by
Keywords: | review removed |
---|
I can't review the necessity of the abort, so someone else should do that.
comment:6 Changed 10 years ago by
Keywords: | review added |
---|
- The NameError has been fixed.
- Fixed.
- 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 10 years ago by
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 10 years ago by
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".
comment:9 Changed 9 years ago by
Keywords: | httpclient review added |
---|---|
Owner: | radix deleted |
comment:10 Changed 9 years ago by
Branch: | → branches/expressive-client-timeouts-3811 |
---|
(In [27665]) Branching to 'expressive-client-timeouts-3811'
comment:11 Changed 9 years ago by
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 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:13 Changed 8 years ago by
Owner: | radix deleted |
---|
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.