Evaluate support for preload and monkey-patching

[tilgovi]

Extending discussion from #927, it would be good if, for R20, we can take a firm stance on whether gevent/eventlet are supported with the preload option. This evaluation will probably depend on our approach to multiprocessing under Windows, as well as the state of gevent/eventlet and how we plan to maintain worker classes going forward.

[komuw]

I think the new gc.freeze api would be of use, more details in the cpython pull request and issue ticket;

1. bpo-31558: Add gc.freeze() python/cpython#3705
2. https://bugs.python.org/issue31558

[hingston]

As of gevent 1.3a1 you get the following RuntimeWarning

/env/lib/python3.6/site-packages/gunicorn/workers/ggevent.py:65: RuntimeWarning: Monkey-patching ssl after ssl has already been imported may lead to errors, including RecursionError on Python 3.6. Please monkey-patch earlier.

[jamadden]

The warning was added as a result of gevent/gevent#1016. Basically, importing requests and then monkey-patching was essentially guaranteed to lead to a RecursionError (which looks like #1559).

[benoitc]

@tilgovi how do you think we can progress on that topic? Would be good to draft some roadmap for it.

In the mean time I would postpone it for the next release if you're OK?

[tilgovi]

I'm okay to postpone. @jamadden do you think we should try importing gevent earlier? What has been your experience? If I remember correctly, I've seen code you shared from a project where you monkey-patch before loading Gunicorn as a custom application.

[jamadden]

@jamadden do you think we should try importing gevent earlier? What has been your experience? If I remember correctly, I've seen code you shared from a project where you monkey-patch before loading Gunicorn as a custom application.

Yes, we use a setuptools console entry point to first monkey-patch, then load gunicorn's own console entry point, thus ensuring that everything is monkey-patched before any application code is pre-loaded. It works very well for us.

[benoitc]

postponing to 20.1 let's try to find a solution for it :)

[benoitc]

So back on this topic since someone reported on irc he hit the error. The problem I see in monkey patching during preload is that it would make the socket unblocked in the arbiter, which is not really wanted. We should make sure to use the unpatched module/function in the arbiter.

[tilgovi]

Does the arbiter perform any blocking calls on the socket? If not, do we care if the socket is patched in the arbiter? Mostly, the arbiter just passes the socket to workers.

[benoitc]

The arbiter itself rely on a blocking behaviour for select and signaling. Also it's safer to bind the socket in blocking mode. This is why it's discouraged to patch outside of a worker. Also in term of design i am thinking it's better to have the arbiter isolated from the application logic.

I am thinking there is a way to make sure the arbiter is always the unpatched module/functions. Kind of patching before the patch happen so w keep original modules available for our own usage. This would allows gevent, eventlet but also any other lib to do whatever they want. cc @jamadden

[tilgovi]

I'm not sure I see a danger anywhere in the way the arbiter uses sockets or signals. If I remember correctly, @jamadden has experience patching the arbiter early with gevent without issue.

[jamadden]

Yes, we still patch before running any gunicorn code and haven't encountered any issues as far as I know. gunicorn master and workers all respond to signal as expected, requests are delivered as expected, etc. We recently enabled SO_REUSEPORT and everything continued to works.

[kvikshaug]

FWIW, our organization patches like this for a half-dozen microservices in production and we have not run into any issues with it.

[benoitc]

I'm not against patching it sooner in the case of preload case. Maybe indeed it won't arm to just patch it, so why not trying it in a branch for the next 2 days (so we can release 20.0.1 on thursday).

I think it requires the following changes:

• add a preload method to the worker that would be called by the arbitert before we preload the WSGI APP. I think we can reuse the setup method for it and add an optionnal argument preload that would be true during preload.
• ensure in the arbiter the worker knows that it is preloading the app.

Do you see anything else?

[tilgovi]

I think we have some minor issues to fix for 20.0.1, but should defer this to 20.1 or 21, if we think it's at all risky. I don't think we should change any APIs, even internal APIs, in a patch release.

[benoitc]

@tilgov agree, the change it planned to the milestone 20.1. Btw There is a 20.0.1 milestone created, so if something is missing please add it to it :)

[LronDC]

So... how is it going with the RuntimeWarning issue

[prakhar1010]

Is supporting preload with monkey-patching planned for any upcoming release ?

[boxed]

There is no planning really. If there is a merged PR it will be in. If someone asks I might make a release.

[benoitc]

no activity since awhile. closing feel free to create a new ticket if needed.