#712 mod_pinger breaks hibernated smacks sessions

Reporter ge0rg
Owner Nobody
Stars ★ (1)  
  • Status-New
  • Priority-Medium
  • Type-Defect
  1. ge0rg on

    mod_pinger periodically tries to ping every connected s2s and c2s connection. It does so even onhibernated smacks sessions, causing the timeout to happen and the session to be destroyed (tested on 0.9 and 0.10): Jul 27 15:03:49 yax.im:pinger info Client "georg@yax.im/resource" silent for too long, closing... Jul 27 15:03:49 c2s554e970 debug Destroying session for georg@yax.im/resource (georg@yax.im): connection-timeout The ideal solution I am looking for should do the following: 1. only ping a connection after no data was received from the other side for a configurable idle timeout (instead of pinging all clients and servers at the same time, irregardless of data received) 2. automatically use <r> instead of <iq ping> on 0198-enabled connections 3. if no <a> or <iq response> was received after another configurable ping timeout, close or hibernate the session All the current solutions have shortcomings regarding at least one of these requirements: - mod_pinger does not honor incoming stanzas, causing more pings than actually needed, and it breaks hibernated sessions - mod_smacks has a handle_read_timeout (that fulfills requirement 1), but it has no support for non-0198 sessions (which is inacceptable) and uses the same timeout value for both 1 and 3 (which I could live with) I'd be fine with this becoming part of smacks or pinger or a separate module, as long as it is properly communicated. Thank you.

  2. ge0rg on

    P.S: mod_pinger is using a hardcoded "urn:xmpp:sm:2" namespace, which will probably break clients. Instead it should use whatever namespace the client has negotiated.

  3. ge0rg on

    P.S: mod_pinger is using a hardcoded "urn:xmpp:sm:2" namespace, which will probably break clients. Instead it should use whatever namespace the client has negotiated.

  4. Thilo Molitor (tmolitor) on

    One could use the 'smacks-hibernation-start' and 'smacks-hibernation-end' events to suspend mod_pinger activities on hibernated sessions. For the namespace issue: I could add a new event 'smacks-activated' which is fired when smacks gets activated for a session and contains the used smacks namespace.

  5. Thilo Molitor (tmolitor) on

    Forget the namespace thing: session.smacks already contains the used namespace!

  6. Thilo Molitor (tmolitor) on

    As far as I can see, all above points are more or less fulfilled. I just fixed the smacks namespace issue. So this can be closed imho.

  7. Thilo Molitor (tmolitor) on

    PS: I also fixed the original issue that mod_pinger didn't stop pinging when the session got hibernated.

  8. ge0rg on

    The code change looks good, I'll test and report back. One thing though: if session.smacks and not session.awaiting_ack then session.send(st.stanza("r", { xmlns = session.smacks })) This is not very good style. If smacks is enabled and an ack is pending, you fall back to IQ-ping. There is no bad in sending a second <r/>, but IMHO there is no need for even that, because if there is a pending one, it will either be responded to anyway, or the ping timeout will trigger.

  9. ge0rg on

    Got this from latest mod_pinger on 0.10: Apr 06 14:22:22 general error Top-level error, please report: ...tc/prosody/prosody-modules/mod_pinger/mod_pinger.lua:9: attempt to index field 'idle_watchdog' (a nil value) Apr 06 14:22:22 general error stack traceback: /usr/bin/prosody:4: in function 'loop' /usr/lib/prosody/net/server_event.lua:751: in function </usr/lib/prosody/net/server_event.lua:750> [C]: in function 'xpcall' /usr/bin/prosody:396: in function 'loop' /usr/bin/prosody:427: in main chunk [C]: ? Really, updating modules and then going to a business appointment is a BAD idea.

  10. Thilo Molitor (tmolitor) on

    @georg: I didn't change the line you got the error from...I can only imagine this happens on smacks hibernation and/or resume, is that right? Well, the logic that the module sends out ping stanzas if waiting for a smacks ack isn't from me, I only changed it from the hardcoded smacks namespace to the dynamic one. But you are right imho, I'll change this :)

  11. Thilo Molitor (tmolitor) on

    @ge0rg: I think I fixed your bug (I used the wrong session in the smacks-hibernation-end event). I also fixed the ping logic to not ping if we're already waiting for an ack as you suggested. See: https://hg.prosody.im/prosody-modules/rev/c971b2cee2cc

New comment