#6167 defect closed fixed (fixed)

Investigate problem with "Connection: Keep-Alive" handling in http.HTTPChannel.checkPersistence

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/http-persistence-6167
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

PersistenceTestCase.testAlgorithm could test this case, but someone commented out the data (first line of ptests class attribute). If uncommented, the test fails.

Someone needs to investigate. Why does it fail? Is it a buggy test or a buggy checkPersistence?

Change History (7)

comment:1 Changed 22 months ago by DefaultCC Plugin

  • Cc jknight added

comment:2 Changed 22 months ago by meissenPlate

From twisted.web.http.HTTPChannel.checkPersistence:

        # HTTP 1.0 persistent connection support is currently disabled, since
        # we need a way to disable pipelining. HTTP 1.0 can't do pipelining
        # since we can't know in advance if we'll have a content-length header,
        # if we don't have the header we need to close the connection. In HTTP
        # 1.1 this is not an issue since we use chunked encoding if
        # content-length is not available.

From twisted.web.test.test_http.PersistenceTestCase:

    ptests = [
        # (_prequest(connection=[b"Keep-Alive"]), b"HTTP/1.0", 1, {b'connection' : [b'Keep-Alive']}),
        (_prequest(), b"HTTP/1.0", 0, {b'connection': None}),
        (_prequest(connection=[b"close"]), b"HTTP/1.1", 0,
         {b'connection' : [b'close']}),
        (_prequest(), b"HTTP/1.1", 1, {b'connection': None}),
        (_prequest(), b"HTTP/0.9", 0, {b'connection': None}),
        ]

This unit test tests what checkPersistence decides in 5 specific cases:

  1. HTTP/1.0 with the Keep-Alive header returns True


  1. HTTP/1.0 with no specific header defaults to False


  1. HTTP/1.1 with the close header returns False


  1. HTTP/1.1 with no specific header defaults to True


  1. HTTP/0.9 with no specific header defaults to False

The first of those tests is commented out, because, as the comment in twisted.web.http.HTTPChannel explains, Twisted doesn't currently implement persistence of HTTP/1.0 connections, because it doesn't handle pipelining. Whatever that is.
http://en.wikipedia.org/wiki/HTTP_pipelining


It's been this way for a LONG TIME.

checkPersistence has been refusing to deal with HTTP/1.0 persistence for at least as long as the SVN repo has been around.

twisted.web.http, 8 years ago, the revision where http was moved to its new home:

http://twistedmatrix.com/trac/browser/trunk/twisted/protocols/http.py?rev=10967

twisted.protocol.http, where HTTPChannel used to live, 11 years ago, and the oldest revision available on the SVN repo:

http://twistedmatrix.com/trac/browser/trunk/twisted/protocols/http.py?rev=1633

(HINT: control F to "checkPersistence(self, ")

The unit test in question has had "(_prequest(connection=[b"Keep-Alive"]), b"HTTP/1.0", 1, {b'connection' : [b'Keep-Alive']})" commented out since it was written 11 years ago.

Here's the original changeset, from 2002, when that test was added:

http://twistedmatrix.com/trac/changeset/1521

(Control F to "class PersistenceTestCase(unittest.T")

Note that the file has moved since then, from twisted.test.test_http to twisted.web.test.test_http. I checked some revisions of test_http over time. I don't think there has EVER been a time when that line WASN'T commented out.


I searched for "http pipelining" in the tracker and checked over a bunch of tickets, and I see nothing pertaining to this issue.

Basically you've stumbled on a TODO that isn't marked, that hasn't been touched in 11 years, and that doesn't have a ticket.

Options:

  1. Find someone who knows what this pipelining stuff is and figure out how to disable it in HTTP/1.0 so we can fix persistence in HTTP/1.0 and then reenable that test.


  1. Delete all the dead code. It's been dead for 11 years, so it probably doesn't matter that much.


Or at least that's my assessment. But, obviously, I don't really know what I'm talking about :).

comment:3 Changed 20 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/http-persistence-6167

(In [36646]) Branching to 'http-persistence-6167'

comment:4 Changed 20 months ago by exarkun

(In [36647]) rewrite the persistence tests to make each case more explicit; delete the dead code that nods at the possibility of http/1.0 persistent connections

refs #6167

comment:5 Changed 20 months ago by exarkun

  • Keywords review added

Thanks! That thorough analysis made it easy to decide what to do here.

I did basically just delete the case which was commented out, as well as the commented out code that might perhaps have been intended to support that case. I also rewrote the comment in checkPersistence in a way that I think makes the intent more clear, and I rewrote the unit test as several different unit tests, one for each case.

Build results

comment:6 Changed 20 months ago by tom.prince

  • Keywords review removed
  • Owner set to exarkun

There is a failure on py3, due to the added line in _prequest. That line looks entirely superfluous, the only call to that function with an argument has neither - nor _ in the name or value. Other than that the changes look fine.

Please commit after fixing and a green buildbot run.

comment:7 Changed 20 months ago by exarkun

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

(In [36721]) Merge http-persistence-6167

Author: meissenPlate, exarkun
Reviewer: tom.prince
Fixes: #6167

Remove the forever-commented-out HTTP/1.0 persistence test, refactor the
remaining portion of that test into several different tests (one for each
case), and try to clarify the comment in the implementation about what is
going on with respect to persistent connections and pre-HTTP/1.1 requests.

Note: See TracTickets for help on using tickets.