#712 mod_pinger breaks hibernated smacks sessions

Reporter Ge0rG
Owner Nobody
Created
Updated
Stars ★ (1)
Tags
  • Status-Fixed
  • Component-Community
  • Priority-Medium
  • Patch
  • 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

  12. Ge0rG on

    It looks like mod_pinger is still destroying sessions, but now differently: There was an incoming <a/> and after just 14s the pinger still killed the session, despite it being active: Aug 18 13:16:42 c2s90860c0 debug Received[c2s]: <a xmlns='urn:xmpp:sm:2' h='863'> Aug 18 13:16:42 c2s90860c0 debug #queue = 0 Aug 18 13:16:56 yax.im:pinger info Client "georg@yax.im/yaxim" silent for too long, closing... Aug 18 13:16:56 c2s90860c0 debug Disconnecting client, <stream:error> is: <stream:error><connection-timeout xmlns='urn:ietf:params:xml:ns:xmpp-streams'/></stream:error> Aug 18 13:16:56 c2s90860c0 debug c2s stream for georg@yax.im/yaxim closed: connection-timeout Aug 18 13:16:56 c2s90860c0 debug Destroying session for georg@yax.im/yaxim (georg@yax.im): connection-timeout Aug 18 13:16:56 c2s90860c0 debug Handled 140 incoming stanzas Aug 18 13:16:56 c2s90860c0 debug Received[c2s]: <presence type='unavailable'> Aug 18 13:16:56 c2s90860c0 info Client disconnected: connection closed Aug 18 13:16:56 c2s90860c0 debug Destroying session for (unknown) ((unknown)@(unknown)) Then, this is repeated multiple times: Aug 18 13:17:38 c2s90860c0 debug mod_smacks hibernation timeout reached... Aug 18 13:17:38 c2s90860c0 debug The session has already been destroyed Aug 18 13:18:17 c2s90860c0 debug mod_smacks hibernation timeout reached... Aug 18 13:18:17 c2s90860c0 debug The session has already been destroyed

  13. Ge0rG on

    Addendum: I'm using `network_settings.read_timeout = 840` so I could well live with mod_pinger completely ignoring 0198 sessions (as outlined in the first comment to this issue).

  14. Thilo Molitor (tmolitor) on

    what version of the smacks module and what version of the pinger are you using?

  15. Ge0rG on

    As this is a production server, I'm not updating prosody-modules too often: changeset: 2726:55f3ab952d06 tag: tip user: Matthew Wild <mwild1@gmail.com> date: Thu Jul 06 10:48:39 2017 +0100

  16. Zash on

    Changes
    • tags Component-Community
  17. Ge0rG on

    Okay, so I wrote a "fix" for mod_pinger that excludes smacks-enabled sessions, and it got merged: https://op-co.de/tmp/0001-Neuter-0198-from-mod_pinger-fix-712.patch Unfortunately it's broken and causes stack traces. The following is a workaround, please merge as well or fix differently: https://op-co.de/tmp/0002-mod_pinger-work-around-updates-on-stale-sessions.patch

  18. Zash on

    Changes
    • tags Status-Fixed Patch

New comment

Not published. Used for spam prevention and optional update notifications.