Opened 5 years ago

Closed 4 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
(diff, github, buildbot, log)
Author: radix Launchpad Bug:

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 5 years ago by radix

  • Keywords review added

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.

comment:2 Changed 5 years ago by radix

  • Owner jknight deleted

comment:3 Changed 5 years ago by ivank

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

comment:4 Changed 5 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 test_newclient.py

comment:5 Changed 5 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 5 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 5 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 5 years ago by 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".

comment:9 Changed 5 years ago by exarkun

  • 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 5 years ago by radix

  • Branch set to branches/expressive-client-timeouts-3811

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

comment:11 Changed 5 years ago by exarkun

  • 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 4 years ago by exarkun

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

(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 4 years ago by <automation>

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