Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gaiohttpy.py invalid syntax warning (19.0.0) #788

Closed
hyao opened this issue Jun 14, 2014 · 46 comments
Closed

gaiohttpy.py invalid syntax warning (19.0.0) #788

hyao opened this issue Jun 14, 2014 · 46 comments

Comments

@hyao
Copy link

hyao commented Jun 14, 2014

Thanks for the new release!

Just pip installed gunicorn 19.0.0, it installs all right, but there's the following warning during installation (Python 2.7.6):

File "/usr/local/lib/python2.7/dist-packages/gunicorn/workers/gaiohttp.py", line 67
        yield from self.wsgi.close()
                 ^
    SyntaxError: invalid syntax

Thanks,

@hyao hyao changed the title gaihttpy.py invalid syntax warning (19.0.0) gaiohttpy.py invalid syntax warning (19.0.0) Jun 14, 2014
@benoitc
Copy link
Owner

benoitc commented Jun 14, 2014

asyncio requires python 3.4 afaik. Not sure what is required to have trollius but any patch is welcome. Maybe we should test the python version and raise an error first though...

cc @fafhrd91 @asvetlov

@hyao
Copy link
Author

hyao commented Jun 14, 2014

or we could make asyncio package a requirement for versions other than python 3.4?

@berkerpeksag
Copy link
Collaborator

or we could make asyncio package a requirement for versions other than python 3.4?

That would work only for Python 3.3 since tulip works on 3.3+.

@benoitc
Copy link
Owner

benoitc commented Jun 14, 2014

yeah like @berkerpeksag said, asyncio requires Python 3.3 or later: https://pypi.python.org/pypi/asyncio/3.4.1

For python2.7 trollius may do the job but it will require some patching.

@benoitc
Copy link
Owner

benoitc commented Jun 14, 2014

Sorry need more coffee... reverted the commit since it's was not needed. The error is a syntax error happening during install. It is harmless. (

@benoitc
Copy link
Owner

benoitc commented Jun 14, 2014

@hyao if you need its support on python 2.7 with trollius please open a new ticket. Though I strongly suggest you to go on python3 so you won't rely on an hack.

@hyao
Copy link
Author

hyao commented Jun 14, 2014

@benoitc I'm just looking at this from a user's perspective, a syntax error during installation makes the user wonder whether the package is installed correctly or not.

As to use python3 for gaiohttp functionality, thanks for the suggestion. I guess it's already been documented somewhere.

Thanks,

@asvetlov
Copy link
Collaborator

  1. The warning raised on compiling .py files stage.
    To suppress it we need to switch from find_packages in setup.py to manually collecting list of modules. I can take a look on that. No library code should be changed.
  2. aiohttp doesn't support trollius and probably will not in the near future, sorry.

@asvetlov
Copy link
Collaborator

gaiohttp depends on aiohttp, not asyncio itself.
gunicorn has no requires for tornado, gevent, eventlet etc.
That's why I did not add aiohttp into gunicorn requirements.

@berkerpeksag
Copy link
Collaborator

To suppress it we need to switch from find_packages in setup.py to manually collecting list of modules.

This is more like a distutils issue/feature. distutils compiles all Python files to bytecode when installing a package.

@benoitc
Copy link
Owner

benoitc commented Jun 14, 2014

@hyao I agree we should fix it if possible :)

@asvetlov +1 for the changes. While we are here maybe we could remove the setuptools dependency...

@tilgovi
Copy link
Collaborator

tilgovi commented Jun 16, 2014

It's actually easy. We can switch on PY2 and use trollius yield From(...) instead of the yield from ... syntax.

@tilgovi
Copy link
Collaborator

tilgovi commented Jun 16, 2014

It won't do any good unless aiohttp works on PY2 this way, also.

@fafhrd91
Copy link
Collaborator

aiohttp is not compatible with trolius

@benoitc
Copy link
Owner

benoitc commented Jun 16, 2014

Unless someone will propose a patch to aiohttp, i think having the worker running on python 3.4 and sup is enough for me. Like said @asvetlov we should fix the packaging to remove the warning. Any other idea?

@tilgovi
Copy link
Collaborator

tilgovi commented Jun 17, 2014

No other idea. I think that's right. I could make a trollius support PR to aiohttp, but until that happens there's nothing but packaging to avoid the warning we can do in gunicorn.

@fafhrd91
Copy link
Collaborator

to support trolius in aiohttp you have to rewrite it

@tilgovi
Copy link
Collaborator

tilgovi commented Jun 17, 2014

to support trollius in aiohttp you have to rewrite it

I don't think that's true. https://trollius.readthedocs.org/#differences-between-trollius-and-tulip

@fafhrd91
Copy link
Collaborator

As an author of aiohttp, I can say that's true, you have to rewrite it. Aiohttp uses yield from internally and it uses yield from not as coroutines.

@tilgovi
Copy link
Collaborator

tilgovi commented Jun 17, 2014

I'm certainly not trying to challenge your knowledge of your own library, but I'm skeptical and think "rewrite" might be hyperbole.

In case it's useful, here are the places where "yield from" is used outside a coroutine.

parsers.py

LinesParser
ChunksParser

websocket.py

WebSocketParser
parse_frame
parse_message

protocol.py

HttpPrefixParser
HttpRequestParser
HttpResponseParser
HttpPayloadParser

connector.py

TCPConnector

multidict.py

_ValuesView

But there are also a few places where send() and throw() are used with subgenerators, within the connector, protocol, and parser.

So it wouldn't be no work. It may be too much mess that you wouldn't want it. But it is probably achievable and I don't think it would be rewriting everything.

@fafhrd91
Copy link
Collaborator

of cause it is possible to make it work with trolius, but does it worth effort? trolius helps you with coroutines but all of of this generators (except connector.py) are not coroutines, so basically you need to remove "yeild from" and replace with something else, what is rewrite to me.

i tried to refactor parser in https://github.com/KeepSafe/aiohttp/tree/parsers_refactoring branch, but then abandoned it. to refactor current implementation requires rethinking whole architecture.

but PR is welcome! :)

@tilgovi
Copy link
Collaborator

tilgovi commented Jun 17, 2014

Cool :)

I probably won't give it a try. Just recording notes. Thanks! I'm excited
to try the aio worker.
On Jun 16, 2014 7:03 PM, "Nikolay Kim" [email protected] wrote:

of cause it is possible to make it work with trolius, but does it worth
effort? trolius helps you with coroutines but all of of this generators
(except connector.py) are not coroutines, so basically you need to remove
"yeild from" and replace with something else, what is rewrite to me.

i tried to refactor parser in
https://github.com/KeepSafe/aiohttp/tree/parsers_refactoring branch, but
then abandoned it. to refactor current implementation requires rethinking
whole architecture.

but PR is welcome! :)


Reply to this email directly or view it on GitHub
#788 (comment).

@benoitc benoitc reopened this Jun 20, 2014
@benoitc
Copy link
Owner

benoitc commented Jun 20, 2014

I reopened the issue. We should fix the packaging issue.

@asvetlov
Copy link
Collaborator

Will do it at weekend

@benoitc
Copy link
Owner

benoitc commented Jun 20, 2014

@asvetlov thanks!

@asvetlov
Copy link
Collaborator

Please take a look on PR #801

@schmitch
Copy link

I also got some issues with gaiohttp.

Here is my log:
Stacktrace (aktuellster Aufruf als letztes):

File "django/core/handlers/base.py", line 104, in get_response
response = middleware_method(request, callback, callback_args, callback_kwargs)
File "django/middleware/csrf.py", line 170, in process_view
request_csrf_token = request.POST.get('csrfmiddlewaretoken', '')
File "django/core/handlers/wsgi.py", line 135, in _get_post
self._load_post_and_files()
File "django/http/request.py", line 244, in _load_post_and_files
self._post, self._files = QueryDict(self.body, encoding=self._encoding), MultiValueDict()
File "django/http/request.py", line 205, in body
self._body = self.read()
File "django/http/request.py", line 259, in read
return self._stream.read(_args, *_kwargs)
File "django/core/handlers/wsgi.py", line 53, in read
result = self.buffer + self._read_limited()
File "django/core/handlers/wsgi.py", line 47, in _read_limited
result = self.stream.read(size)

Only gaio will give these errors.

Python 3.4 imo.

The gthread worker, works fine. He is blocking the requests a little bit more than I could accept, so I will stay on 2.7 with gevent.

@asvetlov
Copy link
Collaborator

@c-schmitt Sorry but I don't see neither exception nor gaiohttp code in logged stack trace.
Please elaborate your problem.

@schmitch
Copy link

@asvetlov it does, maybe it isn't clear but i will explain.
These lines are django code. self.stream.read(size) will take the stream from wsgi.input enviorn which will be set bei aiohttp in here: https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/wsgi.py#L51 The payload will be set here:
https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/server.py#L186

And here comes the problem. It only accepts read() not read(size).
But it is specified here: http://legacy.python.org/dev/peps/pep-0333/#input-and-error-streams
that all PEP333 servers needs to have these methods.
So gaiohttp or aiohttp is missing this method. Which is really bad for production code.

Edit:

The methods listed in the table above must be supported by all servers conforming to this specification. Applications conforming to this specification must not use any other methods or attributes of the input or errors objects. In particular, applications must not attempt to close these streams, even if they possess close() methods.

So gunicorn won't be compatible with pep333 if we don't fix that on aiohttp side or just mark gaio as not compatible with pep333.

Edit2:
Even the spec linked bei aiohttp, needs to have read(n):
http://legacy.python.org/dev/peps/pep-3156/#servers

@asvetlov
Copy link
Collaborator

@c-schmitt Understood. Thank you.
Will fix the issue ASAP.

@asvetlov
Copy link
Collaborator

After conversation in aio-libs/aiohttp#81 I think the objection from @c-schmitt is not an issue of gunicorn.

Anyway, if somebody think that problem should be fixed -- please open new issue.
This one is about gunicorn packaging, not about aiohttp behavior.
@benoitc @fafhrd91 @c-schmitt -- thoughts?
I've close this one, all related work should be done #801 PR.

@c-schmitt would you attach reasonable simple the code sample which reproduces your problem (in new issue)?
I'd like to figure out is this one your specific issue or any django code cannot work with gaiohttp worker.

@benoitc
Copy link
Owner

benoitc commented Jun 23, 2014

hrm the wsgi spec says that read(n) should be implemented:

http://legacy.python.org/dev/peps/pep-3333/#input-and-error-streams

@asvetlov reading the issue on aiohttp why read(n) couldn't be implemented?

@fafhrd91
Copy link
Collaborator

i think nobody expect django to work on top of asyncio out of box.
it is possible to make WSGIServerHttpProtocol full wsgi compliant with readpayload=True, but does anyone want to load full payload into memory in production?

@schmitch
Copy link

@asvetlov gunicorn needs to meet the wsgi spec.

gunicorn 'Green Unicorn' is a WSGI HTTP Server for UNIX, fast clients and sleepy applications.
http://www.gunicorn.org

Else you could rename it to another thing, but don't call it WSGI.
That aiohttp don't want to meet their spec isn't a problem. But gunicorn shouldn't support it, if workers can't meet pep-333.

@fafhrd91
Copy link
Collaborator

@c-schmitt i'm ok with that.

@koobs
Copy link

koobs commented Feb 25, 2015

@asvetlov I'm still seeing this in 19.2.1, but for _gaiohttp.py

  File "/usr/local/lib/python2.7/site-packages/gunicorn/workers/_gaiohttp.py", line 68
    yield from self.wsgi.close()
             ^
SyntaxError: invalid syntax

@benoitc
Copy link
Owner

benoitc commented Feb 25, 2015

@koobs which version of gaiohttp did you installed?

@koobs
Copy link

koobs commented Feb 25, 2015

@benoitc The version of aiohttp was 0.9.0, but its unrelated to the issue. I just updated the FreeBSD port to 0.14.4 anyway for the sake of it, just to isolate.

The real issue is that on Python 2.7 (when aiohttp isnt even installed), compilation of _gaiohttp.py is attempted during build/build-install and that fails, leaving pyc/pyo files missing, but referenced in --record output.

@koobs
Copy link

koobs commented Feb 25, 2015

@benoitc More precisely, this was likely fixed in 86f7404 (unless it didnt also cover _aiohttp.py) but then regressed with 719e61b

@benoitc
Copy link
Owner

benoitc commented Feb 25, 2015

@koobs err I mean, are you using the latest version of pip? It should be fixed there. Let me know.

@berkerpeksag
Copy link
Collaborator

Perhaps we can add a small note to the documentation?

@benoitc
Copy link
Owner

benoitc commented Feb 25, 2015

@berkerpeksag waiting the split I think It's a good idea :) We should start the split of workers and other plugins asap to release a new version in march.

@matrixise
Copy link
Collaborator

Yesterday, I just installed the last version, pip install gunicorn and I see this error syntax, for me, first thing, I think there is a problem I uninstall this version and try to work with a previous version.

After that, I have read the error and understood that I can use it because it's just a "warning" for pip.

First reaction of a user, uninstall it and use an older version :/

@berkerpeksag
Copy link
Collaborator

@benoitc yes, good idea. I need to read #837 again :)

@catskul
Copy link

catskul commented Apr 14, 2015

Still seeing it in 19.3.0

@berkerpeksag
Copy link
Collaborator

@catskul please update your pip version. The warning is not harmful and recent pip versions don't show it anymore.

uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Dec 27, 2015
- Update PORTVERSION and distinfo checksum (19.4.1)
- Match COMMENT to setup.py:description=
- Add LICENSE_FILE
- Options: Rename SETPROC option -> Add PROCTITLE to defaults
- Options: Add GAIOHTTP async worker
- Options: Update all descriptions
- Options: Remove TESTS optionjs
- Enable "concurrent" Python support
- Add NO_ARCH (architecture independent package)
- Rename test target to new conventions
- Check, limit, and add BROKEN message for Option/Python version
  combinations that aren't supported.
- Patch out non-compulsory dependencies from test requirements file
- Delete requirements_dev.txt patch file (no longer necessary)

- Add post-patch: target to remove _gaiohttp.py, because compileall() of this file
  fails on 2.x with a SyntaxError, but is not handled, so .py[co] files references
  are added to --record output, breaking file list generation. [1][2][3][4][5]

Changes:

  http://docs.gunicorn.org/en/stable/news.html

[1] benoitc/gunicorn#788
[2] benoitc/gunicorn#860
[3] benoitc/gunicorn#982
[4] pypa/pip#1873
[5] pypa/pip#1954


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@404558 35697150-7ecd-e111-bb59-0022644237b5
koobs added a commit to freebsd/freebsd-ports that referenced this issue Dec 27, 2015
- Update PORTVERSION and distinfo checksum (19.4.1)
- Match COMMENT to setup.py:description=
- Add LICENSE_FILE
- Options: Rename SETPROC option -> Add PROCTITLE to defaults
- Options: Add GAIOHTTP async worker
- Options: Update all descriptions
- Options: Remove TESTS optionjs
- Enable "concurrent" Python support
- Add NO_ARCH (architecture independent package)
- Rename test target to new conventions
- Check, limit, and add BROKEN message for Option/Python version
  combinations that aren't supported.
- Patch out non-compulsory dependencies from test requirements file
- Delete requirements_dev.txt patch file (no longer necessary)

- Add post-patch: target to remove _gaiohttp.py, because compileall() of this file
  fails on 2.x with a SyntaxError, but is not handled, so .py[co] files references
  are added to --record output, breaking file list generation. [1][2][3][4][5]

Changes:

  http://docs.gunicorn.org/en/stable/news.html

[1] benoitc/gunicorn#788
[2] benoitc/gunicorn#860
[3] benoitc/gunicorn#982
[4] pypa/pip#1873
[5] pypa/pip#1954
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants