brack3t / Djrill

[INACTIVE/UNMAINTAINED] Djrill is an email backend and new message class for Django users that want to take advantage of the Mandrill transactional email service from MailChimp.
BSD 3-Clause "New" or "Revised" License
319 stars 64 forks source link

Inbound webhook decorators not triggering #109

Closed Koed00 closed 8 years ago

Koed00 commented 8 years ago

We've been running inbound webhooks via djrill decorators for quite some time now. When suddenly they stopped working. Currently the signals are not triggering the decorated webhook functions, but the webhook does return a status 200. Because of this the problem took a while to get noticed.

Unfortunately I can't retrace when this happened, so it could be a regression or just an incompatibility with the latest Django version.

Currently broken for Django 1.9.2 and Djrill 2.0.

medmunds commented 8 years ago

Hmm, the webhook signal receiver tests are passing in all supported versions, including Django 1.9. And Djrill doesn't return the 200 response until after it sends the signals. So if you're seeing the status 200, Djrill has tried to notify any registered receivers (and none raised any exceptions).

How and where are you defining your signal receiver? If it's not at the top level of a module, python may be garbage collecting it before Djrill gets a chance to call it. (And that behavior would be unpredictable and could be affected by unrelated changes to your code.) See Django's weak option for listening to signals for more info.

If that's not the issue, I'm not sure what's going wrong. It would probably help to see your receiver code.

Koed00 commented 8 years ago

Your explanation seems to fit with what I'm seeing. When debugging I can see the signals going off, but the receiver functions aren't being called. Maybe the problem is that I have multiple receivers set up. I'll see what moving them around in the module or disabling them does.

medmunds commented 8 years ago

Multiple receivers should be fine.

If your receivers are local functions (defined inside some other function), try using the weak=False option when you connect them to the webhook_event signal. Example:

class SomeExampleView(View):
    def some_example_method(self):
        # The handle_inbound function below is local to the some_example_method function,
        # so must be connected using the weak=False option to avoid garbage collection:
        @receiver(webhook_event, weak=False) # <<<<<<<
        def handle_inbound(sender, event_type, data, **kwargs):
            if event_type == "inbound": ...
medmunds commented 8 years ago

Multiple receivers should be fine.

Also, just double-checking: multiple receivers are fine as long as they're different functions. This won't work:

@receiver(webhook_event)
def my_receiver(sender, event_type, data, **kwargs):  # this will never be called
    if event_type == 'inbound':
        print "inbound"

@receiver(webhook_event)
def my_receiver(sender, event_type, data, **kwargs):  # because this has the same name
    if event_type == 'hard_bounce':
        print "hard_bounce"

The first my_receiver will never be called, because it gets overwritten by the second definition of my_receiver below it. (It's an easy mistake to make by copy and paste. Python won't warn you about redefining a function like this, but pylint or an IDE like PyCharm should flag it as a problem.)

Koed00 commented 8 years ago

No no, none of that. I'll try again after the weekend. I have a feeling the weak option will probably fix it. Just concerned that it's failing in old , working code after upgrading to Djrill 2 and Django 1.9

medmunds commented 8 years ago

OK -- just wanted to double-check.

And yeah, if your receivers are local functions (which weak=False will solve), then it was really only working by luck before you upgraded. Any change, anywhere, might have altered your memory usage and caused python to garbage-collect your receivers out of existence.

medmunds commented 8 years ago

@Koed00 hoping you were able to resolve this with the weak option. If not, please reopen and include your code that defines and connects the Djrill webhook receivers that aren't getting called.

Koed00 commented 8 years ago

@medmunds sorry, I had a crazy release week.

Yes, for future reference, the weak option solved it for us. Thanks a lot for a great package.