Opened 6 years ago

Closed 6 years ago

#6751 defect closed fixed (fixed)

twisted.web._newclient.Response leaves its transport in an unpredictable state depending on how large the response body is

Reported by: Glyph Owned by: Glyph
Priority: high Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/deliverBody-hang-6751
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description (last modified by Glyph)

If an HTTP response has a small response-body, and a Twisted application does not immediately call deliverBody when said response is received, Response can leave its transport paused.

When I say "immediately", I mean, synchronously within the response callback's invocation. If you call deliverBody at any point later, you go through the _deliverBody_DEFERRED_CLOSE callback which doesn't work.

Among other things, this means that when HTTP11ClientProtocol.quiescentCallback is invoked, the 'quiescent' connection might be reading (if it happened to go through _deliverBody_INITIAL) or it might be paused (if it happened to go through _deliverBody_DEFERRED_CLOSE).

This will lead to two negative effects:

  1. twisted.web.client.HTTPConnectionPool will end up adding paused connections to its persistent connection cache, and
  2. if something manages to be holding on to the transport, it'll silently hang around forever, leaking an FD; if nothing is holding on to it, it'll be GC'd, closing its socket (without calling connectionLost) some time later.

Change History (14)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 6 years ago by Glyph

Description: modified (diff)

comment:3 Changed 6 years ago by Itamar Turner-Trauring

Resolution: duplicate
Status: newclosed

Duplicate of #6711 (but maybe you want to move the description over there, since it's better).

comment:4 Changed 6 years ago by Glyph

Resolution: duplicate
Status: closedreopened

Incorrect. This occurs even if you do call deliverBody.

comment:5 Changed 6 years ago by Glyph

Description: modified (diff)

comment:6 Changed 6 years ago by Glyph

Author: glyph
Branch: branches/deliverBody-hang-6751

(In [41416]) Branching to deliverBody-hang-6751

comment:7 Changed 6 years ago by Glyph

Keywords: review added
Priority: normalhigh
Status: reopenednew

I think the code in the branch should address this. Setting the priority to "high" since this is actually a pretty serious resource leak that can be very confusing to debug.

comment:8 Changed 6 years ago by Itamar Turner-Trauring

What happens when you never call deliverBody? Feel free to say "I opened a different ticket for that", but maybe you could solve it at same time, who knows.

comment:9 in reply to:  8 Changed 6 years ago by Glyph

Replying to itamar:

What happens when you never call deliverBody? Feel free to say "I opened a different ticket for that", but maybe you could solve it at same time, who knows.

So, if you never call deliverBody, the connection never gets added back to the pool.

If the server wants to time you out, it will time you out and the connection will be removed from the pool (and the reactor, and you'll disconnect).

If the server wants to let the connection hang out forever, it can hang out forever.

In other words, now not calling deliverBody is, in a sense, a perfectly valid thing to do, and you'll get exactly the behavior that one would expect from that.

As it happens, never calling deliverBody won't hang the connection pool, because the connection pool just makes sure that there are no more than maxPersistentPerHost idle connections.

Arguably, there should be a way to set a default timeout on how long one should take before calling deliverBody, but that is more of a new feature than a bug fix.

comment:10 Changed 6 years ago by Wilfredo Sánchez Vega

Owner: set to Wilfredo Sánchez Vega

comment:11 Changed 6 years ago by Wilfredo Sánchez Vega

Status: newassigned

comment:12 Changed 6 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: changed from Wilfredo Sánchez Vega to Glyph
Status: assignednew

Reviewed. No suggested changes.

Kicked the Windows builder that ran out of disk space, passed this time. WinXP failed due in unrelated stuff because it's Windows or something.

OK to merge.

comment:13 in reply to:  12 Changed 6 years ago by Glyph

Replying to wsanchez:

Reviewed. No suggested changes.

Thanks a bunch!

Kicked the Windows builder that ran out of disk space, passed this time. WinXP failed due in unrelated stuff because it's Windows or something.

FYI, the Windows builder isn't actually running out of disk space. It's running out of "being a functioning operating system". We could fix this bug in Twisted and man we really should so this stops being a perennial comment in reviews. You can see the ticket describing the problem, which even has a branch already, although is not in review, here.

OK to merge.

OK, will land soon.

comment:14 Changed 6 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [41439]) Merge deliverBody-hang-6751: Fix a hang in Agent when using a connection pool.

Author: glyph

Reviewer: wsanchez

Fixes: #6751

twisted.web.client.Agent now correctly manage flow-control on pooled connections, and therefore requests will no longer hang sometimes when deliverBody is not called synchronously within the callback on Request.

Note: See TracTickets for help on using tickets.