tmolitor-stud-tu / mod_push_appserver

Simple and extendable appserver for XMPP pushes (aka. XEP-0357)
MIT License
25 stars 9 forks source link

docs(readme): mention the incompatibility with mod_pubsub #20

Closed marc0s closed 5 years ago

marc0s commented 5 years ago

mod_push_appserver hooks into the same events that mod_pubsub with the same priority, so once the server is started you'll have no precise knowledge of which module will be handling the matching stanzas.

tmolitor-stud-tu commented 5 years ago

I'd rather like to make them compatible instead of just putting up a warning in the readme.

tmolitor-stud-tu commented 5 years ago

Could you provide me an example prosody log showing the conflict of this modules. From looking at the code I fail to see where these two modules block each other.

tmolitor-stud-tu commented 5 years ago

Does this patch help?

diff --git a/mod_push_appserver/mod_push_appserver.lua b/mod_push_appserver/mod_push_appserver.lua
index 6c9f360..b1934ad 100644
--- a/mod_push_appserver/mod_push_appserver.lua
+++ b/mod_push_appserver/mod_push_appserver.lua
@@ -277,7 +277,7 @@ module:hook("iq/host", function(event)
        -- true indicates handling via async_callback, everything else is synchronous and must be handled directly
        if not (type(success) == "boolean" and success) then async_callback(success); end
        return true;
-end);
+end, 10);

 local function unregister_push_node(node, type)
        local settings = push_store:get(node);
marc0s commented 5 years ago

The problem is that both modules hook iq/host with default priority and when starting up, prosody will pick one handler or another, randomly. I observed this behavior when pushes where not working at all just sometimes, and the behavior was consistent until I restarted. Then, it might work or it might not, depending on which handler won the race :)

marc0s commented 5 years ago

Does this patch help?

diff --git a/mod_push_appserver/mod_push_appserver.lua b/mod_push_appserver/mod_push_appserver.lua
index 6c9f360..b1934ad 100644
--- a/mod_push_appserver/mod_push_appserver.lua
+++ b/mod_push_appserver/mod_push_appserver.lua
@@ -277,7 +277,7 @@ module:hook("iq/host", function(event)
        -- true indicates handling via async_callback, everything else is synchronous and must be handled directly
        if not (type(success) == "boolean" and success) then async_callback(success); end
        return true;
-end);
+end, 10);

 local function unregister_push_node(node, type)
        local settings = push_store:get(node);

I cannot test right now, but this will run this hook because of the higher priority, yes. What will happen to the hook of mod_pubsub then? Will it depend on the return value of this one, right?

tmolitor-stud-tu commented 5 years ago

All priority 0 handlers are invoked but in arbitrary order. If a handler returns something else than nil this indicates that the event is handled and other handlers are not executed.

The problem is to distinguish normal pubsub publish and push publish. My latest commit should solve that while still using priority 0. Can you confirm this?

marc0s commented 5 years ago

The problem is to distinguish normal pubsub publish and push publish. My latest commit should solve that while still using priority 0. Can you confirm this?

I cannot test right now, but looking at the change, it looks good. If there's no notification element nil is returned, so other (mod_pubsub's, presumably) handlers should be run.

I'll report back whenever I can test it.

Thanks :)

tmolitor-stud-tu commented 5 years ago

Closing this one, feel free to reopen it if your tests don't succeed.