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

aiohttp worker #775

Merged
merged 1 commit into from
Jun 9, 2014
Merged

aiohttp worker #775

merged 1 commit into from
Jun 9, 2014

Conversation

fafhrd91
Copy link
Collaborator

@fafhrd91 fafhrd91 commented Jun 7, 2014

@benoitc aiohttp has unittests for worker, i'm not sure you want me to move as well.

@asvetlov
Copy link
Collaborator

asvetlov commented Jun 7, 2014

The patch is LGTM.
I'm +0 for adding tests also.
They are already present, so moving tests from aiohttp into gunicorn is quick.
But I cannot find any tests for other workers: tornado, gevent, eventlet etc.

@benoitc benoitc mentioned this pull request Jun 7, 2014
@asvetlov
Copy link
Collaborator

asvetlov commented Jun 7, 2014

Also I have a question about documentation. Is there enough to mention gaiohttp worker in design.rst and settings.rst?
Also I don't grok structure of news files. Should the change to be mentioned in both news.rst and 2014-news.rst?

@benoitc
Copy link
Owner

benoitc commented Jun 7, 2014

@fafhrd91 @asvetlov we really need to add tests for worker but it can be done later imo( @davisp @tilgovi thoughts?) .

I just tested the worker with the test example and got the following error:

2014-06-07 14:51:04 [4579] [ERROR] Error handling request
Traceback (most recent call last):
  File "/home/benoitc/work/gunicorn/env_py3/lib/python3.4/site-packages/aiohttp/server.py", line 189, in start
    yield from handler
  File "/home/benoitc/work/gunicorn/env_py3/lib/python3.4/site-packages/aiohttp/wsgi.py", line 166, in handle_request
    yield from resp.write(item)
  File "/home/benoitc/work/gunicorn/env_py3/lib/python3.4/site-packages/aiohttp/protocol.py", line 664, in write
    self.send_headers()
  File "/home/benoitc/work/gunicorn/env_py3/lib/python3.4/site-packages/aiohttp/protocol.py", line 633, in send_headers
    hdrs = hdrs.encode('ascii') + b'\r\n'

To fix it i had to comment the following line:

--- test.py 2014-06-02 07:37:42.116918158 +0200
+++ test_asyncio.py 2014-06-07 14:48:20.010819384 +0200
@@ -25,7 +25,7 @@
         ('Content-type','text/plain'),
         ('Content-Length', str(len(data))),
         ('X-Gunicorn-Version', __version__),
-        ("Test", "test тест"),
+        #("Test", "test тест"),
     ]
     start_response(status, response_headers)
     return iter([data])

In the gunicorn.http.wsgi module we are fixing it with value = str(value).strip() and once we are sending the headers string is converted using the function util.to_bytestring. Maybe that could also be fixed in aiohttp?

The rest is OK for me.

@benoitc
Copy link
Owner

benoitc commented Jun 7, 2014

@asvetlov settings.rst is build from the config.py file. So any doc should be added in the module source code. The worker should be advised in design.rst indeed. (It also should be done for the threaded worker).

there is one changelog / year and 1 "current" changelog indeed... I didn't find the time yet to make it automatic :)

@benoitc
Copy link
Owner

benoitc commented Jun 7, 2014

i opened an issue in the aiohttp project with a possible patch. It's expected to have headers as utf8.

@asvetlov
Copy link
Collaborator

asvetlov commented Jun 7, 2014

@benoitc as you can see we have fixed utf8 headers problem on master but we need for other important to aiohttp issues before making new release.
If you can wait for us or if you can publish new gunicorn release tomorrow with current state -- that's fine.
As an option we can cherry-pick the utf-8 patch into 0.7.4 aiohttp bugfix release quickly -- that will not stop you from publishing new gunicorn release.

For documentation changes would do you like to do it yourself?
I'll be unavailable for tomorrow (sorry, I've invited to BBQ party).

If you like the plan -- let me know.

@benoitc
Copy link
Owner

benoitc commented Jun 7, 2014

@asvetlov Thanks! cherry-picking would be the best option if you can't do the other changes next week. I am travelling tomorrow. I will decide on Monday what to do. Will also test the final patch.

@tilgovi @davisp let me know what you think about it anyway :)

@asvetlov
Copy link
Collaborator

asvetlov commented Jun 7, 2014

As I see the point of decision has moved to Monday.
Nikolay @fafhrd91 want to make 0.8 aiohttp release shortly.
Sure, he will do it.
But if aiohttp will still not ready to make new release at point-of-decision we can switch to bugfix release.

Anyway, @benoitc please take care on gunicorn documentation upgrade.

@benoitc benoitc added this to the R19 milestone Jun 8, 2014
@benoitc
Copy link
Owner

benoitc commented Jun 8, 2014

@fafhrd91 @asvetlov cool, let me know for the release. I planned the next gunicorn on friday (with everything done on thursday).

I have created #779 to make sure to not forget about the doc.

@fafhrd91
Copy link
Collaborator Author

fafhrd91 commented Jun 9, 2014

i've just released 0.8.0 version.

@benoitc
Copy link
Owner

benoitc commented Jun 9, 2014

+1 for the merge

asvetlov added a commit that referenced this pull request Jun 9, 2014
@asvetlov asvetlov merged commit fbb2d01 into benoitc:master Jun 9, 2014
@asvetlov
Copy link
Collaborator

asvetlov commented Jun 9, 2014

Tests added in e1d97f1

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

Successfully merging this pull request may close these issues.

3 participants