classification
Title: http.server: minor code style changes.
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.araujo, ezio.melotti, karlcow, maker, orsenthil, python-dev
Priority: normal Keywords: patch

Created on 2011-10-30 14:40 by karlcow, last changed 2012-01-14 07:16 by orsenthil. This issue is now closed.

Files
File name Uploaded Description Edit
issue13294.patch maker, 2011-11-16 10:25 review
issue13294.patch maker, 2011-11-18 12:54 review
Messages (14)
msg146639 - (view) Author: karl (karlcow) * Date: 2011-10-30 14:40
A very simple HTTP server

#!/usr/bin/python3
import http.server
from os import chdir

# CONFIG
ROOTPATH = '/Your/path/'
PORT = 8000

# CODE
def run(server_class=http.server.HTTPServer, server_handler=http.server.SimpleHTTPRequestHandler):
    server_address = ('', PORT)
    httpd = server_class(server_address, server_handler)
    httpd.serve_forever()

class MyRequestHandler(http.server.SimpleHTTPRequestHandler):
    def do_GET(self):
        pass
    
if __name__ == '__main__':
    chdir(ROOTPATH)
    print("server started on PORT: "+str(PORT))
    run(server_handler=MyRequestHandler)


Let's start the server.

% python3 serveur1.py 
server started on PORT: 8000


And let's do a GET request with curl.

% curl -v http://localhost:8000/
* About to connect() to localhost port 8000 (#0)
*   Trying ::1... Connection refused
*   Trying 127.0.0.1... connected
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET / HTTP/1.1
> User-Agent: curl/7.21.4 (universal-apple-darwin11.0) libcurl/7.21.4 OpenSSL/0.9.8r zlib/1.2.5
> Host: localhost:8000
> Accept: */*
> 
* Empty reply from server
* Connection #0 to host localhost left intact
curl: (52) Empty reply from server
* Closing connection #0

The server sends nothing because GET is not defined and I haven't defined anything in case of errors. So far so good. Now let's do a HEAD request on the same resource.

% curl -vsI http://localhost:8000/
* About to connect() to localhost port 8000 (#0)
*   Trying ::1... Connection refused
*   Trying 127.0.0.1... connected
* Connected to localhost (127.0.0.1) port 8000 (#0)
> HEAD / HTTP/1.1
> User-Agent: curl/7.21.4 (universal-apple-darwin11.0) libcurl/7.21.4 OpenSSL/0.9.8r zlib/1.2.5
> Host: localhost:8000
> Accept: */*
> 
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
HTTP/1.0 200 OK
< Server: SimpleHTTP/0.6 Python/3.1.2
Server: SimpleHTTP/0.6 Python/3.1.2
< Date: Sun, 30 Oct 2011 14:19:00 GMT
Date: Sun, 30 Oct 2011 14:19:00 GMT
< Content-type: text/html; charset=utf-8
Content-type: text/html; charset=utf-8
< Content-Length: 346
Content-Length: 346

< 
* Closing connection #0


The server shows in the log the request
localhost - - [30/Oct/2011 10:19:00] "HEAD / HTTP/1.1" 200 -

And is answering.

I would suggest that the default behavior is to have something similar to the one for the GET aka nothing. Or to modify the library code that for any resources not yet defined. The server answers a code 403 Forbidden. 

I could submit a patch in the next few days.
msg147472 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-12 11:10
Hi Karl,
I’m not clear about what problem or need this report describes.  Is it a proposition to add a new method for SimpleHTTPRequestHandler to handle HEAD requests?
msg147479 - (view) Author: karl (karlcow) * Date: 2011-11-12 12:24
Eric,

Two possible solutions to explore:

Either the HEAD reports exactly the same thing than a GET without the body, because it is the role of the GET, but that means indeed adding support for the HEAD. 

or creating a catch-all answer for all unknown or not implemented methods with a "501 Method not implemented" response from the server.

Right now the HEAD returns something :)

I still need to propose a patch. Daily job get into the way :)
msg147493 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-12 14:52
> Right now the HEAD returns something :)
Ah, so this is a bug.

> I still need to propose a patch.
A patch to add a test (for 2.7 or 3.2, see devguide) would be a great first step.
msg147764 - (view) Author: Michele Orrù (maker) * Date: 2011-11-16 10:25
Well, actually SimpleHTTPRequesthandler extends BaseHTTPHandler with basic do_GET and do_HEAD methods.

Unittests for http.server shows that this behavior is intended, since: 
Traceback (most recent call last):
  File "Lib/test/test_httpservers.py", line 639, in <module>
    test_main()
  File "Lib/test/test_httpservers.py", line 633, in test_main
    SimpleHTTPRequestHandlerTestCase,
  File "/Users/maker/dev/cpython/Lib/test/support.py", line 1274, in run_unittest
    _run_suite(suite)
  File "/Users/maker/dev/cpython/Lib/test/support.py", line 1249, in _run_suite
    raise TestFailed(err)
test.support.TestFailed: Traceback (most recent call last):
  File "Lib/test/test_httpservers.py", line 273, in test_head
    self.check_status_and_reason(response, 200)
  File "Lib/test/test_httpservers.py", line 242, in check_status_and_reason
    self.assertEqual(response.status, status)
AssertionError: 501 != 200

So, imho this is not a bug. Anyway, I would propose a trivial patch to make http.server a little more elegant.
msg147847 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-18 10:23
Hi Michele, long time no see :)

> Well, actually SimpleHTTPRequesthandler extends BaseHTTPHandler with basic do_GET and
> do_HEAD methods.
> Unittests for http.server shows that this behavior is intended, since: [snip] 
Not sure what this test shows (maybe because I need coffee :)

> Anyway, I would propose a trivial patch to make http.server a little more elegant.
The first change may do more that you want it to: rstrip without argument will remove all whitespace, but there the code wants to remove only CRLF or LF.  Anyway, changes for the sake of elegance or cleanliness are not done for the sake of it, but usually as a part of a greater refactoring or fix.  We prefer to minimize chances of compatibility breakage by avoiding gratuitous changes.
msg147866 - (view) Author: Michele Orrù (maker) * Date: 2011-11-18 12:54
These tests shows how SimpleHTTPRequestHandler behaves: if the class
contains a do_FOO method, it is called, otherwise error501 is raised.
That's what Karl said with «Or to modify the library code that for any
resources not yet defined.».
Since SimpleHTTPRequestHandler.do_HEAD exists, and this is reflected
in the correspondent unittest, I belive that this is not a bug.

Patch attached. No problem if it's not committed now. I hope that
someone in the noisy list will make a little cleanup one day :)

>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue13294>
> _______________________________________
>
msg147870 - (view) Author: Michele Orrù (maker) * Date: 2011-11-18 13:03
As Ezio just pointed out, strip('\r\n') is still behaves differently from the previous code. Sorry for that.
msg150138 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2011-12-23 09:09
New changeset 2c1720468dbb by Senthil Kumaran in branch '3.2':
Minor code style improvements in http.server suggested in Issue13294.
http://hg.python.org/cpython/rev/2c1720468dbb

New changeset 0466ee1816b1 by Senthil Kumaran in branch 'default':
merge from 3.2.  Minor code style improvements in http.server suggested in Issue13294.
http://hg.python.org/cpython/rev/0466ee1816b1

New changeset 1b1818fee351 by Senthil Kumaran in branch '2.7':
port to 2.7 - Minor code style improvements in http.server suggested in Issue13294.
http://hg.python.org/cpython/rev/1b1818fee351
msg150140 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-12-23 09:17
The original issue was invalid. 
Incorporated Michele Orrù's code style changes into the trunk and other codelines.
msg150244 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-12-25 02:37
The first chunk of the patch (the use of strip) changes the behavior as discussed in the previous comments, so it should probably be reverted.  The rest of the changes are OK.
msg150246 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2011-12-25 06:16
The concern here is if the request line had something like this.

    Method SP Request-URI SP HTTP-Version <ANY_\r_\n_\r\n_Combination>\r\n

The previous behavior would have resulted in 

    Method SP Request-URI SP HTTP-Version <ANY_\r_\n_\r\n_Combination>

That is removing only the final \r\n, whereas the current change would make it

    Method SP Request-URI SP HTTP-Version 

That is removes all the trailing \r\n combination.

BTW, thing to note this, this is only for request line and not the header
lines.  And for request-line, both HTTP 1.0 and HTTP 1.1 spec has this in section
5.1

    5.1  Request-Line

       The Request-Line begins with a method token, followed by the
       Request-URI and the protocol version, and ending with CRLF. The
       elements are separated by SP characters. No CR or LF are allowed
       except in the final CRLF sequence.

       Request-Line = Method SP Request-URI SP HTTP-Version CRLF

Which leads me to believe that, removing all the trailing \r\n is a fine thing
to do and should not be harmful.

Just to augment this with few other things I found while (re-)reading the spec.
This advise is different from Header's trailing whitespace, which is called
Linear White space (LWS).  If the Host Header looks like, e.g.  "Host:
www.foo.com \r\n" (notice the trailing white space), 

According to RFC 2616 (HTTP 1.1), section 4.2 Message Headers:

   The field-content does not include any leading or trailing LWS:
   linear white space occurring before the first non-whitespace
   character of the field-value or after the last non-whitespace
   character of the field-value. Such leading or trailing LWS MAY be
   removed without changing the semantics of the field value.

RFC 1945 (HTTP 1.0), section 4.2 Message Headers does not make such an explicit
statement.

My guess on the former behavior in http/server.py is that it was thought that
Request-Line was following something like section 4.2 on HTTP 1.0 spec and only
the last two characters were removed. But actually, the request-line as per
spec should have only one CRLF as end char. In the Docstring of the
BaseHTTPServer class, there is a mention about preserving the trailing
white-space, but it does not point to any authoritative reference, so I am sure
taking docstring as reference to preserve the behavior is a good idea.

Before dwelling to find the reason, I was thinking if reverting the patch in
2.7 and 3.1 would be a good idea.  But give that change has support from older
specs to new ones, I am inclined to think that leave the change as such
(without reverting) should be fine as well. 

Only if we find a stronger backwards compatibility argument for leaving
trailing \r\n in request-line, then we should remove it in 2.7 and 3.2,
otherwise we can leave it as such.
msg150398 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-12-30 21:40
Just a note: “Incorporated Michele Orrù's code style changes into the trunk and other codelines.”

The policy is to apply only bug fixes and doc fixes and improvements to stable branches, not code cleanups (they’re usually compatible but are just not worth doing—at one point code is released and should stop being improved).  This is not worth reverting now, unless the commit was buggy (I didn’t read all messages), I just wanted to restate the policy to avoid such future code cleanups in bugfix branches.  Cheers
msg151238 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2012-01-14 07:16
Eric - noted the point. :)
History
Date User Action Args
2012-01-14 07:16:53orsenthilsetmessages: + msg151238
2012-01-14 07:08:02orsenthilsetstatus: open -> closed
2011-12-30 21:40:00eric.araujosetmessages: + msg150398
2011-12-25 06:16:54orsenthilsetmessages: + msg150246
2011-12-25 02:37:31ezio.melottisetstatus: closed -> open

messages: + msg150244
2011-12-23 09:17:55orsenthilsetstatus: open -> closed
resolution: fixed
messages: + msg150140

title: http.server: HEAD request should not return a body -> http.server: minor code style changes.
2011-12-23 09:09:58python-devsetnosy: + python-dev
messages: + msg150138
2011-11-18 13:03:06makersetmessages: + msg147870
2011-11-18 12:54:46makersetfiles: + issue13294.patch

messages: + msg147866
2011-11-18 10:23:55eric.araujosetmessages: + msg147847
2011-11-16 10:25:58makersetfiles: + issue13294.patch

nosy: + maker
messages: + msg147764

keywords: + patch
2011-11-12 14:52:02eric.araujosettype: enhancement -> behavior
title: http.server - HEAD request when no resource is defined. -> http.server: HEAD request should not return a body
messages: + msg147493
versions: + Python 2.7, Python 3.2
2011-11-12 12:24:42karlcowsetmessages: + msg147479
2011-11-12 11:10:59eric.araujosetnosy: + eric.araujo

messages: + msg147472
versions: + Python 3.3, - Python 3.1, Python 3.2
2011-10-30 16:54:17ezio.melottisetnosy: + ezio.melotti
2011-10-30 14:40:41karlcowcreate