add support for eventlets _AlreadyHandled object #1406

Merged
merged 5 commits into from Dec 21, 2016

Conversation

Projects
None yet
5 participants
@stefaang
Contributor

stefaang commented Dec 6, 2016

fixes #1210

@Hum4n01d

This comment has been minimized.

Show comment
Hide comment
@Hum4n01d

Hum4n01d Dec 7, 2016

Does this just silence the error?

Hum4n01d commented Dec 7, 2016

Does this just silence the error?

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 7, 2016

Owner
Owner

benoitc commented Dec 7, 2016

@stefaang

This comment has been minimized.

Show comment
Hide comment
@stefaang

stefaang Dec 7, 2016

Contributor

@Hum4n01d The error was caused by an empty wsgi response on disconnect, due to a slightly different implementation of the _AlreadyHandled object. The goal of _AlreadyHandled is to raise a StopIteration exception to close the socket.
@benoitc I reworked this into a wrapper around self.wsgi by overloading self.load_wsgi in the geventlet worker.
I'm not that familiar with the all the eventing projects... but this error was messing up my logs so I wanted a quick fix.

Contributor

stefaang commented Dec 7, 2016

@Hum4n01d The error was caused by an empty wsgi response on disconnect, due to a slightly different implementation of the _AlreadyHandled object. The goal of _AlreadyHandled is to raise a StopIteration exception to close the socket.
@benoitc I reworked this into a wrapper around self.wsgi by overloading self.load_wsgi in the geventlet worker.
I'm not that familiar with the all the eventing projects... but this error was messing up my logs so I wanted a quick fix.

@Hum4n01d

This comment has been minimized.

Show comment
Hide comment
@Hum4n01d

Hum4n01d Dec 7, 2016

I don't totally get what you're saying but please merge it @benoitc

Hum4n01d commented Dec 7, 2016

I don't totally get what you're saying but please merge it @benoitc

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 10, 2016

Owner

@stefaang the change can be simpler imo,

so we check ALREADY_HANDLED there:
https://github.com/benoitc/gunicorn/blob/master/gunicorn/workers/async.py#L104

Instead we could check it using a function is_already_handled that could be overriden by the worker. In the case of Eventlet it would check for both the gunicorn value and the eventlet value. Do you want to make the change?

Owner

benoitc commented Dec 10, 2016

@stefaang the change can be simpler imo,

so we check ALREADY_HANDLED there:
https://github.com/benoitc/gunicorn/blob/master/gunicorn/workers/async.py#L104

Instead we could check it using a function is_already_handled that could be overriden by the worker. In the case of Eventlet it would check for both the gunicorn value and the eventlet value. Do you want to make the change?

@stefaang

This comment has been minimized.

Show comment
Hide comment
@stefaang

stefaang Dec 12, 2016

Contributor

You're right, that's a better approach. I'll get around with another fix.

Contributor

stefaang commented Dec 12, 2016

You're right, that's a better approach. I'll get around with another fix.

@Hum4n01d

This comment has been minimized.

Show comment
Hide comment
@Hum4n01d

Hum4n01d Dec 14, 2016

So can it be merged?

So can it be merged?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 21, 2016

Collaborator

This looks good to me.

Collaborator

tilgovi commented Dec 21, 2016

This looks good to me.

@benoitc benoitc merged commit 9430742 into benoitc:master Dec 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 21, 2016

Owner

merged. Thanks!

Owner

benoitc commented Dec 21, 2016

merged. Thanks!

@Hum4n01d

This comment has been minimized.

Show comment
Hide comment
@Hum4n01d

Hum4n01d Dec 21, 2016

Yay! Now my logs won't be clogged up

Yay! Now my logs won't be clogged up

@qwexvf

This comment has been minimized.

Show comment
Hide comment
@qwexvf

qwexvf Dec 22, 2016

Thank you for this fix <3

qwexvf commented Dec 22, 2016

Thank you for this fix <3

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

add support for eventlets _AlreadyHandled object (#1406)
add support for eventlets _AlreadyHandled - issue 1210 - Response object has no attribute status_code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment