#7993 enhancement closed fixed (fixed)

'twisted.web.wsgi' should be ported to Python 3

Reported by: Peng Xiao Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: web Keywords:
Cc: Branch: branches/wsgi-py3-7993-6
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, adiroiban

Description (last modified by hawkowl)

This module should be importable, functional, and pass the test suite.

Attachments (8)

destroy-the-supermoon.patch (2.8 KB) - added by jMyles 22 months ago.
destroy-the-supermoon2.patch (5.0 KB) - added by jMyles 22 months ago.
wsgi-python3-7993-3.patch (42.9 KB) - added by Gavin Panella 19 months ago.
wsgi-python3-7993-3.1.patch (59.5 KB) - added by Gavin Panella 19 months ago.
wsgi-python3-7993-5.1.patch (60.0 KB) - added by Gavin Panella 19 months ago.
Same as wsgi-python3-7993-3.1.patch but with dist3.py entries.
wsgi-python3-7993-6.1.patch (16.2 KB) - added by Gavin Panella 19 months ago.
Incremental patch to the wsgi-py3-7993-6 branch which relaxes some type checks in Python 2.
wsgi-python3-7993-6.2.patch (5.4 KB) - added by Gavin Panella 18 months ago.
Second incremental patch to the wsgi-py3-7993-6 branch; apply after wsgi-python3-7993-6.1.patch.
wsgi-python3-7993-6.3.patch (10.7 KB) - added by Gavin Panella 18 months ago.
Third incremental patch to the wsgi-py3-7993-6 branch; apply after wsgi-python3-7993-6.2.patch.

Download all attachments as: .zip

Change History (44)

comment:1 Changed 23 months ago by hawkowl

Component: coreweb
Milestone: Python-3.x
Summary: python3 ImportError: No module named 'twisted.web.wsgi''twisted.web.wsgi' should be ported to Python 3
Type: defectenhancement

Hi,

This is because WSGI support isn't yet ported. I'll amend this ticket to match the others and attach it to the milestone.

comment:2 Changed 23 months ago by hawkowl

Description: modified (diff)

comment:3 Changed 23 months ago by hawkowl

Description: modified (diff)

comment:4 Changed 22 months ago by jMyles

I'm not sure how to translate this assertion:

https://github.com/twisted/twisted/blob/trunk/twisted/web/test/test_wsgi.py#L1190

The tracebacks are no longer equal because the exception is now raised by twisted.python.compat.reraise.

Changed 22 months ago by jMyles

Attachment: destroy-the-supermoon.patch added

comment:5 Changed 22 months ago by jMyles

Keywords: review added

comment:6 Changed 22 months ago by hawkowl

Description: modified (diff)

comment:7 Changed 22 months ago by Adi Roiban

Keywords: review removed
Owner: set to jMyles

Hi,

Your patch includes this code # raise excInfo[0] which should be either removed or uncommented.

Please note that in order for the patch to be accepted it should include changes into dist3.py file, to enable tests and declare that the module is ported on python3.

The patch also needs a newsfile fragment so that the release notes will mention this change. More details here http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

The test should pass on both python2 and python3.

Please consider changing the dist3.py and adding a newsfile and resubmit the patch.

Let me know if you need any help or guidance with the Twisted dev process.

Many thanks for your contribution!

Changed 22 months ago by jMyles

comment:8 Changed 22 months ago by jMyles

I'm feeling pretty good about the progression here. The hendrix test suite runs against this code, as does hendrix itself. There's still some weirdness around the postpath of the Request object (unicode objects are sneaking in; hawkowl tells me it's supposed to be bytes).

The main blocker right now though is this strange error:

builtins.AttributeError: twisted.web.test.test does not exist

comment:9 Changed 22 months ago by Adi Roiban

Author: adiroiban
Branch: branches/py3-t.web-wsgi-7993

(In [45613]) Branching to py3-t.web-wsgi-7993.

comment:10 Changed 22 months ago by Adi Roiban

Please note that your patch should include a news file fragment as described here http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles. In this way the release notes will include info about the wsgi port.

In test_wsgi the traceback should be imported in the lower part of the files together with the other imports from stdlib.


Here are the build results https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/py3-t.web-wsgi-7993

the attribute error /test not found can occur on import errors in the test.

You ran run just the single test file as:

$ ./bin/trial --coverage twisted/web/test/test_wsgi.py

[ERROR]
Traceback (most recent call last):
  File "/home/adi/chevah/twisted/twisted/trial/runner.py", line 784, in loadByName
    return self.suiteFactory([self.findByName(name, recurse=recurse)])
  File "/home/adi/chevah/twisted/twisted/trial/runner.py", line 685, in findByName
    return self.loadFile(_name, recurse=recurse)
  File "/home/adi/chevah/twisted/twisted/trial/runner.py", line 655, in loadFile
    module = SourceFileLoader(name, fileName).load_module()
  File "<frozen importlib._bootstrap>", line 539, in _check_name_wrapper
    
  File "<frozen importlib._bootstrap>", line 1591, in load_module
    
  File "<frozen importlib._bootstrap>", line 596, in _load_module_shim
    
  File "<frozen importlib._bootstrap>", line 1220, in load
    
  File "<frozen importlib._bootstrap>", line 1200, in _load_unlocked
    
  File "<frozen importlib._bootstrap>", line 1129, in _exec
    
  File "<frozen importlib._bootstrap>", line 1448, in exec_module
    
  File "<frozen importlib._bootstrap>", line 321, in _call_with_frames_removed
    
  File "twisted/web/test/test_wsgi.py", line 12, in <module>
    from urllib import quote
builtins.ImportError: cannot import name 'quote'

twisted/web/test/test_wsgi.py
---------------------------

You can use the coverage tool to make sure all ported code has as least some tests.

Not sure why there are Unicode objects in the Request. Please write some test to reproduce that behaviour and then we can investigate the problems.

I hope that the mystery around the blocker was not solved and that you can continue working on this.

Please resubmit a new patch.

Let me know if you need help.

Many thanks for your work!

comment:11 Changed 19 months ago by hawkowl

Author: adiroibanhawkowl, adiroiban
Branch: branches/py3-t.web-wsgi-7993branches/wsgi-py3-7993-2

(In [46343]) Branching to wsgi-py3-7993-2.

Changed 19 months ago by Gavin Panella

Attachment: wsgi-python3-7993-3.patch added

comment:12 Changed 19 months ago by Gavin Panella

Keywords: review added
Owner: jMyles deleted

I've ported t.w.wsgi to Python 3 and read PEP-3333 a couple of times in the process. I started with the work that jMyles had done; thank you for that. This work is already being used in MAAS trunk but other issues related to MAAS's Python 3 port mean I don't want to give an unequivocal works-for-me just yet, though there are no counter indicators.

comment:13 Changed 19 months ago by hawkowl

Branch: branches/wsgi-py3-7993-2branches/wsgi-py3-7993-3

(In [46463]) Branching to wsgi-py3-7993-3.

comment:14 Changed 19 months ago by hawkowl

(In [46464]) applying patch from allenap, refs #7993

comment:15 Changed 19 months ago by hawkowl

Keywords: review removed
Owner: set to Gavin Panella

Hi allenap!

Thanks a bunch for the patch! It's looking quite good, and I ran it on the buildbots -- there seems to be a few introduced lines with lack of branch coverage (see the coverage report at https://codecov.io/github/twisted/twisted/twisted/web/wsgi.py?ref=24366866b20c4d5c36a3c4b33678dae298311043 ).

Would you be able to add additional tests to cover these cases and bring it up to 100%?

Thanks again!

Changed 19 months ago by Gavin Panella

Attachment: wsgi-python3-7993-3.1.patch added

comment:16 Changed 19 months ago by Gavin Panella

Keywords: review added
Owner: Gavin Panella deleted

Tests added. Lots :) It was a good think you prompted me to add those because I had made a few mistakes in the type checks. Please take a look at the latest patch.

comment:17 Changed 19 months ago by Adi Roiban

Branch: branches/wsgi-py3-7993-3branches/wsgi-py3-7993-4

(In [46509]) Branching to wsgi-py3-7993-4.

comment:18 Changed 19 months ago by Adi Roiban

Branch: branches/wsgi-py3-7993-4branches/wsgi-py3-7993-5

(In [46511]) Branching to wsgi-py3-7993-5.

comment:19 Changed 19 months ago by Adi Roiban

Keywords: review removed
Owner: set to Gavin Panella

I have applied the latest patch and sent it to buildbot.

One ubuntu builder fails on GTK tests, but I think this is unrelated to this branch.

I think that wsgi code is not enabled on py3 https://codecov.io/github/twisted/twisted/twisted/web/wsgi.py?ref=29ac80098241308102028d5c1bef73eba90b63a1

Please check and submit a new patch on top of latest branch.

Thanks!

Changed 19 months ago by Gavin Panella

Attachment: wsgi-python3-7993-5.1.patch added

Same as wsgi-python3-7993-3.1.patch but with dist3.py entries.

comment:20 Changed 19 months ago by Gavin Panella

Keywords: review added
Owner: changed from Gavin Panella to Adi Roiban

New patch with missing dist3.py entries reinstated.

comment:21 Changed 19 months ago by Adi Roiban

Branch: branches/wsgi-py3-7993-5branches/wsgi-py3-7993-6

(In [46517]) Branching to wsgi-py3-7993-6.

comment:22 Changed 19 months ago by Adi Roiban

Latest patch applied and sent to buildbot.

Thanks!

comment:23 Changed 19 months ago by Adi Roiban

Owner: Adi Roiban deleted

Since I have never used wsgi I find it hard to do this review.

If there is no one else to review it, I will start reading about wsgi and try to do a review.

thanks!

comment:24 Changed 19 months ago by Gavin Panella

Keywords: review removed
Owner: set to Gavin Panella

I'm going to change the outcome of some of the type checks after some discussion on #twisted-dev. Slightly edited transcript:

<allenap> adiroiban: The type checks I've added do follow PEP-3333 but
  /may/ break non-compliant WSGI apps in Python 2. Should we try to be
  bug-compatible or to stay close to the spec?
<glyph> bug-compatible
<glyph> most WSGI implemenations are _made_ of bugs
<glyph> allenap: feel free to emit a *warning* or something when the
  implementation is non-compliant
<allenap> glyph: Okay. In Python 3 should it promote those warnings to
  exceptions? In many cases the type checks catch issues that are going
  to cause fires a moment later. Catching them early and raising a more
  informative error seems like a win, or it is better to stick with
  warnings all round?
<allenap> glyph: There's one type check that we might want to keep: in
  _WSGIResponse.write() the write is handed off via callFromThread(), so
  exceptions don't propagate back into the WSGI app's context (there's
  also no back-pressure, but that's a separate issue).
<glyph> allenap: would blockingCallFromThread be a better idea?
<glyph> allenap: I'm not sure. You shouldn't just warn and then
  something you *know* is wrong (for example, if you are receiving
  something you expect to be bytes, and you got unicode instead, you
  shouldn't notice that it's unicode, warn about it, and pass it along
  so you'll get a less-useful traceback a moment later)
<glyph> allenap: if you're going to emit a warning, then also do a
  workaround
<glyph> the previous discussion made me think that there were
  non-compliant applications that might have previously worked
<allenap> glyph: blockingCallFromThread would be good, and would provide
  back-pressure. I'll do that.
<allenap> I'll update my patch with the rest of that advice too.

Changed 19 months ago by Gavin Panella

Attachment: wsgi-python3-7993-6.1.patch added

Incremental patch to the wsgi-py3-7993-6 branch which relaxes some type checks in Python 2.

comment:25 Changed 19 months ago by Gavin Panella

Keywords: review added
Owner: Gavin Panella deleted

Hiya. Please have a look at my latest patch (wsgi-python3-7993-6.1.patch​, apply on top of the wsgi-py3-7993-6​ branch) which:

  • Relaxes header type checks in both Python 2 and Python 3. Previously headers that were not plain lists containing 2-tuples would be rejected. Now any sequence is allowed for both, but warnings are issued when things are not in strict compliance with PEP-3333. Non-sequence types still elicit a TypeError.
  • Writing a non-native string to the wsgi.errors stream in Python 3 is still an error, but in Python 2 it elicits only a warning.
  • The status string passed to start_response() MUST be a native string on both Python 2 and 3. It'll be rejected by Twisted's HTTP machinery in either case, so this rejects it here in order to provide a better error message.
  • Same story for response header keys and values.
  • _WSGIResponse.write() now uses blockingCallFromThread(). This doesn't provide back-pressure as I hoped it might (which I could have realised earlier, since writes to transports don't block and don't return Deferreds) but it does mean that errors from the transport will be propagated back into the WSGI app's thread.

Previously, WSGI apps could write indefinitely to, say, a disconnected client.

comment:26 Changed 19 months ago by Adi Roiban

Thanks.

I have applied your patch and sent it to buildbot

Here is my thin review.

Leaving it on the review queue so that someone with a good understand of wsgi can check it.

Thanks!


Major comments

I would say that test_wsgiErrorsAcceptsOnlyNativeStrings should be split in 2 tests each test with its own docsting.

---

For me, having different tests with the same docstring is a code smell.

Maybe instead of "SHOULD be a tuple" we can update the docstring to say that "For bug-compatiblity reasons, 2 element lists are also accepted." ... or something like this to describe what functionality is covered by the test.

I feel that the comments inside the new tests should be part of the docstring.


Minor comments

All this new type checking and format checking looks complicated :) Is it that critical if an instance which looks like a 2 element sequence is passed ? ... or a sequnce is passed instead of a list. Maybe it is, so feel free to ignore my question.

For test_headersShouldBePlainList do we need inlineCallbacks ? Do we really have an async call there? Can we use getSuccessResultOf() ?

Thanks!

Changed 18 months ago by Gavin Panella

Attachment: wsgi-python3-7993-6.2.patch added

Second incremental patch to the wsgi-py3-7993-6 branch; apply after wsgi-python3-7993-6.1.patch.

comment:27 Changed 18 months ago by Gavin Panella

I would say that test_wsgiErrorsAcceptsOnlyNativeStrings should be split in 2 tests each test with its own docsting.

Done; please see wsgi-python3-7993-6.2.patch.

For me, having different tests with the same docstring is a code smell.

Maybe instead of "SHOULD be a tuple" we can update the docstring to say that "For bug-compatiblity reasons, 2 element lists are also accepted." ... or something like this to describe what functionality is covered by the test.

I feel that the comments inside the new tests should be part of the docstring.

Good points, all done.

All this new type checking and format checking looks complicated :) Is it that critical if an instance which looks like a 2 element sequence is passed ? ... or a sequnce is passed instead of a list. Maybe it is, so feel free to ignore my question.

I think they're valuable. Though the checks are now more relaxed than PEP-3333 expects, they will help people write more compliant and more portable WSGI apps, and will help diagnose problems when moving from other containers.

We could be very relaxed and trust in duck-typing. My experience in general and with porting Twisted and non-Twisted code to Python 3 is that this can make development much harder. Assumptions (like: this is 2-element tuple, or: this is a native string) are often made in places far away from the code being developed, causing unexpected errors. It can be a long process to unwind the steps that arrived at that place, and thus to find the root cause. The type checks I've put in will, in some cases, help make these kinds of bugs shallower.

They're also about being responsible in the wider Python community. Put another way, Twisted ought to stick to the PEPs where possible. If every WSGI container reinterpreted PEP-3333, for example, its usefulness as a standard would be diluted.

Lastly, the checks now only raise exceptions where it is known that breakage is guaranteed, e.g. passing a non-native string for the response status. In other situations, where duck-typing may work, only warnings are issued for non-compliance with the PEP.

For test_headersShouldBePlainList do we need inlineCallbacks ? Do we really have an async call there? Can we use getSuccessResultOf() ?

I know of extract_result() from testtools, but I can't find any reference to getSuccessResultOf(). You're right though, something like that would work.

Changed 18 months ago by Gavin Panella

Attachment: wsgi-python3-7993-6.3.patch added

Third incremental patch to the wsgi-py3-7993-6 branch; apply after wsgi-python3-7993-6.2.patch.

comment:28 Changed 18 months ago by hawkowl

(In [46531]) apply patches wsgi-python3-7993-6.2.patch and wsgi-python3-7993-6.3.patch by allenap, refs #7993

comment:29 Changed 18 months ago by hawkowl

There are possibly some backwards compat issues with going to PEP-3333 -- I've applied for a compat exception on the ML, so we can enforce the PEP properly.

Thanks for your continued work, allenap, it's really appreciated.

Last edited 18 months ago by hawkowl (previous) (diff)

comment:30 Changed 18 months ago by hawkowl

Keywords: review removed
Owner: set to hawkowl

With some more reading with tomprince, this patch seems to be far more backwards compatible than I thought it was, from when I read it last -- the potential edge cases seem to be if you are doing really wacky things, which is why I did the compat exception -- I would rather we were more explicit that this would break things that worked by accident (eg. giving a 3-tuple of headers for some reason), but not break things which are maybe not the cleanest in regards to Unicode on Python 2.

So, other than getting signoff + waiting a week to make sure it doesn't break anyone's code that comes forward, I think this is good to land.

comment:31 Changed 18 months ago by Glyph

(Reminder that it's good to land.)

comment:32 Changed 17 months ago by hawkowl

Resolution: fixed
Status: newclosed

(In [46634]) Merge wsgi-py3-7993-6: Port twisted.web.wsgi to Python 3

Authors: jMyles, allenap Reviewers: adiroiban, hawkowl Fixes: #7993

Special thanks to tomprince and glyph for review assistance.

comment:33 Changed 15 months ago by uspike

Resolution: fixed
Status: closedreopened

Hi,

web/tap.py does not seem to be updated. FIXME points to this ticket: https://github.com/twisted/twisted/blob/616a7422eee0053f69002a878e2f317bc8aaa63c/twisted/web/tap.py#L23

Because the import is not run on python3, twistd web --wsgi does not work on python 3 with 16.1 or trunk code.

comment:34 Changed 15 months ago by Adi Roiban

Many thanks for the report.

This code was already released so I don't think that it helps to re-open and revert it in trunk.

I think that is better to have a separate ticket and work on adding twisted wsgi functionalities.

Also, we might want to look at updating the twistedchecker to raise a warning when we are working on a ticket and we have not addressed all the FIXME comments for that ticket id.

comment:35 Changed 15 months ago by uspike

Thanks for the prompt response. It seems to me that all functionality has been added except for removing the python2 only imports in web/test/test_tap.py and web/tap.py which block twistd --wsgi functionality. By removing the if not _PY3 fence, everything seems to work fine including unit tests. I'd gladly open a new ticket and submit a patch for this issue if that is preferred.

comment:36 Changed 15 months ago by Adi Roiban

Resolution: fixed
Status: reopenedclosed

For me it make sense to just think that #7993 did the wsgi heavy lifting and consider the job done and create a new ticket to take care of twistd integration.

Please create a new ticket + patch + test which checkes that wsgi is available in twisted in py3

Thanks!

Note: See TracTickets for help on using tickets.