Run listeners using subprocess running in background

[tarekziade]

This pull request adds the following:

• Adds a registry.workers class that's a Pool of processes.
• Makes the notification system uses the workers to run asynchronously
• Refactor the code to use Python 3.4's enum.Enum

[tarekziade]

related change: Kinto/kinto#435

[tarekziade]

workaround for pypy, see uqfoundation/dill#73

[tarekziade]

Signals will be in their own PR #648

[Natim]

I don't like to start discussion like that when code is already written and I really would like this kind of implementation to start with an article that explain the rationale. I may have missed the discussion around that though.

What I understood of our plan is defined here: http://www.servicedenuages.fr/en/notifications-kinto-preamble It was the outcome of a really long discussion we had in Douarnenez about how to handle notifications.

I am not really fond of the idea of spawning process from the web server. This means that each wsgi worker will have its own pool of memory workers:

• It adds complexity to the web server code
• It means that a async worker crash will make the web worker crash and that we will probably have to add code to monitor workers and add new ones later (using signals) Part of this code is already handled by supervisord, gunicorn or circus.

The plan we had in mind was to use Redis to add tasks to a queue and then have worker that will consume this queue.

If I understand correctly, your plan with this code is to feed the Redis queue with async calls made by your worker.

The way it works currently is IMO much simpler because a worker is always handling a request or triggering a notification.

If we make sure that the notification handling happens after the response was sent back to the user then we can be confident that it will handle the notification asynchronously before the worker taking another request.

Because we are using uwsgi if a request arrive in the mean time another web worker will be spawn so that the notification task(s) will not block other requests.

Am I missing the point?

[Natim]

avoid

[leplatrem]

(?)

Quickly, some related discussions:

• Remove synchronous example in notifications tutorial Kinto/kinto#420
• make all notifications async Kinto/kinto#422

[tarekziade]

As an irregular contributor, I have initiated some discussions and opened a bunch of issues here and there to make sure I was doing everything right and I have discussed the rationale with @leplatrem several times. I am happy to explain thing to @Natim but I am not going to continue working on this feature: getting back a feedback where my PR is rejected because I did not write an article about it is a very negative signal - it does not make me want to contribute code to the project - we should make sure this does not happen for contributors.

If it's mandatory to write an article prior to a change, please make it clear in the contributing document. (and write an article about this change before?? :) . FWIW I think it's a weird process. We should write articles on new features we added in Kinto. Feedback on design should be done in an interactive system that's meant for it: the issue tracker . Feedback on code should be done in the PRs.

[Natim]

I agree with you that it should not be mandatory to write an article prior to any change in the code base.
I see writing an article more like a way to start the discussion, expose the rationale and agree on the path forward before implementing it. But it is totally fine to start the discussion with some code instead :)

You apparently had this discussion last week and I missed it.

Your concerns about listener code taking time during a notification is really a good point and we should fix that.

We can see for instance that activating cliquet-pusher adds up to the resources response time.
Same for the send_email tutorial.

I am sorry if my comment made you think you should close your pull-request, it wasn't meant to be. We need to fix that and your pull-request aims to do so.

I was just asking myself out-loud if handling workers in the WSGI worker was the right solution and maybe it is.

I would love to discuss this next week so that we can agree on the solution to fix the issue you are pointing out.

[almet]

So I believe the plan was to land this as-is, with minor tweaks, but I do not remember exactly what. @leplatrem @Natim can you add some more info here?

[Natim]

As far as I understood the idea was to mark listener as async or sync in the configuration and being able to select the async backend we'd like to use to queue using this kinto-async-memory-backend plugin as a default async backend.

In the future other async queing system such as Celery or rq could also be plugged as async listener.

@leplatrem is that what you had in mind too?

[leplatrem]

My ideas:

• Be sure we are super clear with the vocabulary in the glossary: workers (background) versus listeners (subscribers from settings) versus subscribers (raw pyramid subscribers)
• Mimic the pluggability of other backends using a setting to specify workers class cliquet.background.workers = cliquet.workers.memory
• Add an async parameters on listeners (default: true)
• If no listener is async, then do not instantiate the workers (nit)
• Wait for Distinguish before/after events #651 to be merged, and plug listeners to AfterResourceChanged

[Natim]

Wait for #651 to be merged, and plug listeners to AfterResourceChanged

Just async onces maybe. This should be configurable at the listener configuration level.

Add an async parameters on listeners (default: true)

I would rather go for default sync true so that we do not break compatibility on listerner configuration.

Also it is always a good question to ask yourself if the notification should be async or sync and if you want a failing notification to break the transaction or not.

In the case of OneCRL if we cannot send a mail, do we want to mark the collection in a review state while nobody was notified?

[leplatrem]

• Make test pass when listener crashes
• Test that if process is recycled when listener crashes
• Use maxtasksperchild to kill the process when it has run 100 tasks (to avoid memory growth)
• Kill the process when listener times out. See https://gist.github.com/tarekziade/365099a010589aadb405