wsgiref problems & quibbles

Hi, Phillip,

so I threw together a few WSGI apps recently, and used
wsgiref to serve
them.  I ran into two problems, and had a few quibbles to
raise as well.

===

First, a patch to fix the existing unit tests is attached.

===

Second, there's a bug in
handlers.BaseHandler.finish_response, line 132.
The line:

        if not self.result_is_file() and not
self.sendfile():

should read

        if not self.result_is_file() or not self.sendfile():
	                             ^^

Otherwise use of wsgi.file_wrapper fails to return any data.

I also can't find any place in the code where 'sendfile'
is called to
actually send the file.  Should that 'if' statement have
an 'else'??

Patch attached, along with 'break-wsgiref.py'.

===

And now for the few miscellaneous quibbles ;), mostly in re
WSGIServer:

  * the __init__ function is inherited, ultimately, from
  	SocketServer.TCPServer via BaseHTTPServer.  This gives it
three
	uglification problems:

	  - the host, port are passed in as a tuple;
	  - the request handler must be explicitly specified;
	  - the WSGI app object must be set elsewhere.

	i.e. to instantiate WSGIServer and set it up, you need to
call
	  
	  s = wsgiref.simple_server.WSGIServer((host, port),
	                          
wsgiref.simple_server.RequestHandler)
	  s.set_app(app)

        This seems *very* counterintuitive and overcomplex
to me, and
	it seems to be entirely for the benefit of complying with
an
	interface inherited from code that WSGI newbies probably
won't
	be using anyway.

	My suggestion is to eliminate this complexity and simply
make it

	  s = wsgiref.WSGIServer(host, port, app)

	or (if you still insist on the performance enhancement of
	keeping it under a separate module ;)

	  s = wsgiref.simple_server.WSGIServer(host, port, app)

	What do you think?

  * the get_app/set_app functions must be due to support for
python
	2.1; they're obviously unnecessary now, what with
properties in 2.2
	and up.  Should they be changed to properties, for
inclusion
	in the stdlib?

  * the simple_server code is completely untested by the
unit tests,
	it seems.  Do you have any thoughts on how to instrument it
to
	be tested?
	
	(The easiest way for me to test it would be to install
	wsgi_intercept and use urllib2 to run various mock HTTP
tests.
	I doubt people want wsgi_intercept in the stdlib, tho, even
	if it's only used for tests...)

  * finally, there are a lot of blank lines loitering about
in your
  	python files.  Some of them are for code spacing, some of
them
	are just hanging out at the bottom of files like
	simple_server.py.  Do you want to leave these in?

Apologies for the minor quibbles, but I'm trying to take
the chance to
go over the module *before* it becomes (more or less) fixed
in stone...!

cheers,
--titus
Index: src/wsgiref/tests/test_handlers.py
============================================================
=======
--- src/wsgiref/tests/test_handlers.py	(revision 2131)
+++ src/wsgiref/tests/test_handlers.py	(working copy)
 -170,7
+170,7 
 
         stdpat = (
             r"HTTP/%s 200 OK\r\n"
-            r"Date: \w \w [ 0123]\d
\d\d:\d\d:\d\d \d\r\n"
+            r"Date: \w, \d\d \w \d
\d\d:\d\d:\d\d \w\r\n"
             r"%s" r"Content-Length:
0\r\n" r"\r\n"
         )
         shortpat = (
 -203,6
+203,7 
                             (stdpat%(version,sw),
h.stdout.getvalue())
                         )
 
+
 TestClasses = (
     HandlerTests,
 )
Index: src/wsgiref/handlers.py
============================================================
=======
--- src/wsgiref/handlers.py	(revision 2131)
+++ src/wsgiref/handlers.py	(working copy)
 -129,7
+129,7 
         in the event loop to iterate over the data, and to
call
         'self.close()' once the response is finished.
         """
-        if not self.result_is_file() and not
self.sendfile():
+        if not self.result_is_file() or not
self.sendfile():
             for data in self.result:
                 self.write(data)
             self.finish_content()
_______________________________________________
Web-SIG mailing list
Web-SIGpython.org
Web SIG: http://www.python.
org/sigs/web-sig
Unsubscribe: http://mail.python.org/mailman/options/web-sig/bo
nd%40yahoo.com

Titus Brown wrote:
> 	or (if you still insist on the performance enhancement
of
> 	keeping it under a separate module ;)
> 
> 	  s = wsgiref.simple_server.WSGIServer(host, port,
app)

Shouldn't that be wsgiref.simpleserver, to make it fit PEP
8?


-- 
Ian Bicking  /  ianbcolorstudy.com  /  http://blog.ianbicking.org

_______________________________________________
Web-SIG mailing list
Web-SIGpython.org
Web SIG: http://www.python.
org/sigs/web-sig
Unsubscribe: http://mail.python.org/mailman/options/web-sig/bo
nd%40yahoo.com

At 11:54 PM 5/3/2006 -0700, Titus Brown wrote:
>Hi, Phillip,
>
>so I threw together a few WSGI apps recently, and used
wsgiref to serve
>them.  I ran into two problems, and had a few quibbles
to raise as well.
>
>===
>
>First, a patch to fix the existing unit tests is
attached.

Thanks.


>===
>
>Second, there's a bug in
handlers.BaseHandler.finish_response, line 132.
>The line:
>
>         if not self.result_is_file() and not
self.sendfile():
>
>should read
>
>         if not self.result_is_file() or not
self.sendfile():
>                                      ^^
>
>Otherwise use of wsgi.file_wrapper fails to return any
data.

This has been changed in the SVN version now; thanks.


>I also can't find any place in the code where
'sendfile' is called to
>actually send the file.  Should that 'if' statement
have an 'else'??

No.  See the docstring for the 'sendfile()' method. 
'self.sendfile()' is 
supposed to send the file (if it can) and return True,
otherwise False if 
it can't actually send the file.  The default
implementation returns False.


>And now for the few miscellaneous quibbles ;), mostly in
re WSGIServer:
>
>   * the __init__ function is inherited, ultimately,
from
>         SocketServer.TCPServer via BaseHTTPServer. 
This gives it three
>         uglification problems:
>
>           - the host, port are passed in as a tuple;
>           - the request handler must be explicitly
specified;
>           - the WSGI app object must be set elsewhere.
>
>         i.e. to instantiate WSGIServer and set it up,
you need to call
>
>           s = wsgiref.simple_server.WSGIServer((host,
port),
>                                   
wsgiref.simple_server.RequestHandler)
>           s.set_app(app)
>
>         This seems *very* counterintuitive and
overcomplex to me, and
>         it seems to be entirely for the benefit of
complying with an
>         interface inherited from code that WSGI newbies
probably won't
>         be using anyway.
>
>         My suggestion is to eliminate this complexity
and simply make it
>
>           s = wsgiref.WSGIServer(host, port, app)
>
>         or (if you still insist on the performance
enhancement of
>         keeping it under a separate module ;)
>
>           s = wsgiref.simple_server.WSGIServer(host,
port, app)
>
>         What do you think?

I think that to avoid breaking compatibility with existing
programs that 
use WSGIServer I could see supporting
WSGIServer((host,port),app), (with a 
default app of None) but not (host, port, app) unless
another constructor 
was added.


>   * the get_app/set_app functions must be due to
support for python
>         2.1; they're obviously unnecessary now, what
with properties in 2.2
>         and up.

Actually, I don't remember now why I did that.  I'm not
even sure that 
simple_server supports Python 2.1, although most of the
other wsgiref 
modules are intended to support it.


>   Should they be changed to properties, for inclusion
>         in the stdlib?

I don't see a benefit to doing so.  The internal interface
between the 
handler and server objects is such that get_app() is used
now, and of 
course existing code using wsgiref uses set_app().  New code
is likely to 
use the constructor instead, so I don't see any benefit
coming from adding 
properties.


>   * the simple_server code is completely untested by
the unit tests,
>         it seems.  Do you have any thoughts on how to
instrument it to
>         be tested?

Well, for unit testing purposes it seems to me that the
request handler can 
be tested without having an actual connection.  Similarly,
you can unit 
test the server's setup_environ() and other methods added. 
(I'm assuming 
here that "unit" testing would mean testing that
modicum of functionality 
which wsgiref adds to the BaseHTTP stuff.)


>         (The easiest way for me to test it would be to
install
>         wsgi_intercept and use urllib2 to run various
mock HTTP tests.

That sounds more like you want integration testing; I don't
really see a 
way to do that.


>         I doubt people want wsgi_intercept in the
stdlib, tho, even
>         if it's only used for tests...)

Why not?  Is there something wrong with it?  


>   * finally, there are a lot of blank lines loitering
about in your
>         python files.  Some of them are for code
spacing, some of them
>         are just hanging out at the bottom of files
like
>         simple_server.py.  Do you want to leave these
in?

I'd like to, but Guido wouldn't.    I'll let
them die a natural death 
once the code is in the stdlib.  (All of them, including the
trailing ones, 
are to space out code to nicely paged screenfuls in my text
editor, so I 
can navigate easily.)

_______________________________________________
Web-SIG mailing list
Web-SIGpython.org
Web SIG: http://www.python.
org/sigs/web-sig
Unsubscribe: http://mail.python.org/mailman/options/web-sig/bo
nd%40yahoo.com