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

[exarkun]

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?

[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
2. HTTP/1.0 with no specific header defaults to False
3. HTTP/1.1 with the close header returns False
4. HTTP/1.1 with no specific header defaults to True
5. 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.
2. 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 :).

[exarkun]

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

[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

[exarkun]

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

[tom.prince]

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.

[exarkun]

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