plone / plone.recipe.zope2instance

zc.buildout recipe to setup and configure a Zope 2 instance.
https://pypi.org/project/plone.recipe.zope2instance
7 stars 23 forks source link

add new option `asyncore_use_poll` #189

Closed petschki closed 1 year ago

petschki commented 1 year ago

See discussion here https://github.com/plone/plone.recipe.zope2instance/issues/188 . Added option to switch waitress to use poll instead of default select

tschorr commented 1 year ago

Maybe somebody who has the problem in #188 could test whether this works for them, @cillianderoiste , @yurj ?

petschki commented 1 year ago

@petschki other boolean options use on|off rather than/in addition to true|false in buildout.cfg. See e.g. http_fast_listen.

Interesting ... I took the line from clear_untrusted_proxy_headers which defaults to false in the wsgi config templates ... there are several more wsgi options which defaults to true/false so I'm not event sure if there's a common sense of boolean option usage inside this recipe.

nevertheless the config parser of waitress is quite forgiving in boolean values: https://github.com/Pylons/waitress/blob/main/src/waitress/adjustments.py#L30 ...

tschorr commented 1 year ago

... there are several more wsgi options which defaults to true/false so I'm not event sure if there's a common sense of boolean option usage inside this recipe.

There may be some inconsistencies, but when I look in the README, there seem to be a lot of boolean options that use on|off. So people will expect this to work.

nevertheless the config parser of waitress is quite forgiving in boolean values: https://github.com/Pylons/waitress/blob/main/src/waitress/adjustments.py#L30 ...

Maybe, but your current implementation will translate asyncore_use_poll = on from buildout.cfg to asyncore_use_poll = false in wsgi.ini, which is definitely not what we want.

petschki commented 1 year ago

Maybe, but your current implementation will translate asyncore_use_poll = on from buildout.cfg to asyncore_use_poll = false in wsgi.ini, which is definitely not what we want.

Please note that the buildout.cfg options are always (well, also not consistent) with dashes instead of underscores so asyncore-use-poll = true in buildout.cfg will get asyncore_use_poll = true in wsgi.cfg now... but I don't mind changeing the boolean value to on|off though ...

tschorr commented 1 year ago

@petschki this is not about underscores. It is about on in buildout.cfg being translated to false in wsgi.ini. It should translate to true. E.g. you could look at https://github.com/plone/plone.recipe.zope2instance/blob/6eed9162220e385ae9053a81d10405708036a6d0/src/plone/recipe/zope2instance/recipe.py#L240 and the following 4 lines as an example.

petschki commented 1 year ago

@petschki this is not about underscores. It is about on in buildout.cfg being translated to false in wsgi.ini. It should translate to true. E.g. you could look at

https://github.com/plone/plone.recipe.zope2instance/blob/6eed9162220e385ae9053a81d10405708036a6d0/src/plone/recipe/zope2instance/recipe.py#L240 and the following 4 lines as an example.

asyncore-use-poll = on (if you've used dashes correctly) was translated to asyncore_use_poll = on which is also true for waitress ... but nevermind. I've now translated on|true always to true ... all other values are false.

petschki commented 1 year ago

totally agree about the mess .... we won't clean it up here 😅

mauritsvanrees commented 1 year ago

I have released plone.recipe.zope2instance 6.12.0 with this, and am including it in Plone 6.0.1 today.

Note that I released Plone 5.2.11 yesterday, so it does not have this new version. But the master branch is used on both coredev 6.0 and 5.2, so individual sites are free to use this version on 5.2 as well.