New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for XEP-0198: Stream Management #166

Merged
merged 12 commits into from May 6, 2014

Conversation

Projects
None yet
5 participants
@weiss
Member

weiss commented Apr 14, 2014

This is a complete implementation of XEP-0198 (Stream Management) for c2s connections. It adds four c2s options (all documented in doc/guide.tex):

stream_management: true|false (default: true)
resume_timeout: Seconds (default: 300)
resend_on_timeout: true|false (default: false)
max_ack_queue: Size (default: 500)

We've been using this patch in production for a while now, and it seems to work fine for us. I tested it against various clients and corner cases.

I've also tried to keep the patch as non-intrusive as possible. While it has to modify some of the existing code, a quick git diff might look worse than it is: There's lots of whitespace changes in ejabberd_c2s:terminate/3 (as the existing code is wrapped into a new case statement); and the c2s #state is updated in some places, so the #state variable names were modified.

If you decide to merge this and any bugs pop up, I'd be happy to look into them.

weiss added some commits Mar 12, 2014

Add initial XEP-0198 support (EJAB-532)
Implement partial support for XEP-0198: Stream Management.  After
successful negotiation of this feature, the server requests an ACK for
each stanza transmitted to the client and responds to ACK requests
issued by the client.  On session termination, the server re-routes any
unacknowledged stanzas.  The length of the pending queue can be limited
by setting the "max_ack_queue" option to some integer value (default:
500).  XEP-0198 support can be disabled entirely by setting the
"stream_management" option to false (default: true).

So far, stream management is implemented only for c2s connections, and
the optional stream resumption feature also described in XEP-0198 is not
(yet) supported.

This addition was originally based on a patch provided by Magnus Henoch
and updated by Grzegorz Grasza.  Their code implements an early draft of
XEP-0198 for some previous version of ejabberd.  It has since been
rewritten almost entirely.
Remove some commented out code
The code that had been commented out at some earlier point in time would
now break XEP-0198.
Support XEP-0198 session resumption
Implement the optional session resumption feature described in XEP-0198.
A client that supports this feature may now resume the previous session
(within a configurable number of seconds) if the connection was lost.
During resumption, ejabberd will retransmit any stanzas that hadn't been
acknowledged by the client.
Remove "fun" element from c2s #state
Memory consumption wise, local "fun" references are quite expensive.
XEP-0198: Bounce unacked stanzas by default
If the new "resend_on_timeout" option is set to false (which it is by
default), bounce any unacknowledged stanzas instead of re-routing them.
XEP-0198: Don't bounce/resend forwarded messages
On connection timeout, drop any messages that were forwarded by some
encapsulating protocol, such as XEP-0280 carbon copies or XEP-0313
archive messages.  Bouncing or resending them could easily lead to
unexpected results.
@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Apr 15, 2014

Works without issues so far (tested in production as well), please merge

runcom commented Apr 15, 2014

Works without issues so far (tested in production as well), please merge

@cthulhuplush

This comment has been minimized.

Show comment
Hide comment
@cthulhuplush

cthulhuplush Apr 24, 2014

Hello there,

I'm getting a failed unexpected-request any time I try to resume the stream, any idea of what might happen?

Thanks for your time.

cthulhuplush commented Apr 24, 2014

Hello there,

I'm getting a failed unexpected-request any time I try to resume the stream, any idea of what might happen?

Thanks for your time.

@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Apr 24, 2014

Member

I'm getting a failed unexpected-request any time I try to resume the stream, any idea of what might happen?

This would happen, for example, if you tried to resume the stream either before authentication or after resource binding. Of course, I can't really tell without looking at the relevant parts of the stream.

Are you in the process of adding XEP-0198 support to some client yourself? If so, I'm happy to help if I can! But maybe we should discuss this elsewhere unless you suspect an issue with my patch.

Member

weiss commented Apr 24, 2014

I'm getting a failed unexpected-request any time I try to resume the stream, any idea of what might happen?

This would happen, for example, if you tried to resume the stream either before authentication or after resource binding. Of course, I can't really tell without looking at the relevant parts of the stream.

Are you in the process of adding XEP-0198 support to some client yourself? If so, I'm happy to help if I can! But maybe we should discuss this elsewhere unless you suspect an issue with my patch.

@cthulhuplush

This comment has been minimized.

Show comment
Hide comment
@cthulhuplush

cthulhuplush Apr 24, 2014

You are absolutely right that this is not the place to discuss this. I will delete both this and my previous message tomorrow.

I will check what you just said and I will contact you through email, if you don't mind of course.

cthulhuplush commented Apr 24, 2014

You are absolutely right that this is not the place to discuss this. I will delete both this and my previous message tomorrow.

I will check what you just said and I will contact you through email, if you don't mind of course.

@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Apr 24, 2014

Member

I will delete both this and my previous message tomorrow.

A few comments won't hurt anyone, just wanted to make sure we don't start a longish discussion on a client implementation here.

I will check what you just said and I will contact you through email, if you don't mind of course.

Sure! holger@jabber.fu-berlin.de will work both as an email address and as an XMPP address.

Member

weiss commented Apr 24, 2014

I will delete both this and my previous message tomorrow.

A few comments won't hurt anyone, just wanted to make sure we don't start a longish discussion on a client implementation here.

I will check what you just said and I will contact you through email, if you don't mind of course.

Sure! holger@jabber.fu-berlin.de will work both as an email address and as an XMPP address.

weiss added some commits Apr 27, 2014

XEP-0198: Turn some warnings into info messages
Don't log warnings on events that will happen during normal operation.
XEP-0198: Log message when waiting for resumption
Log an informational message when a session goes into the pending state
(waiting for resumption) after the connection was lost.  Administrators
may well be interested in this state change when looking into issues.
XEP-0198: Accept stream elements in pending state
Due to timing issues, ejabberd_c2s might receive stream elements from
the client while the session is waiting for stream resumption.  Those
elements are now accepted.
XEP-0198: Don't log protocol issues
There are corner cases where certain clients acknowledge more stanzas
than they received.  Nothing really bad will happen in those cases, and
server administrators can't do anything about such issues anyway.
XEP-0198: Use "mgmt_" prefix for all #state fields
Prefix all ejabberd_c2s #state fields that are used for stream
management with "mgmt_".
Merge remote-tracking branch 'processone/master' into xep-0198
Conflicts:
	doc/guide.tex
	src/ejabberd_c2s.erl

zinid added a commit that referenced this pull request May 6, 2014

Merge pull request #166 from weiss/xep-0198
Add support for XEP-0198: Stream Management

@zinid zinid merged commit 47f627e into processone:master May 6, 2014

@weiss

This comment has been minimized.

Show comment
Hide comment
@weiss

weiss Nov 30, 2015

Member

@nectarbits, I would give up the requirement of having the presence status of mobile devices updated instantly, as that's plain impossible with a TCP-based protocol. You could use a very short ping interval of course, but that will drain your users' batteries.

You might want to read a related FAQ entry.

Member

weiss commented Nov 30, 2015

@nectarbits, I would give up the requirement of having the presence status of mobile devices updated instantly, as that's plain impossible with a TCP-based protocol. You could use a very short ping interval of course, but that will drain your users' batteries.

You might want to read a related FAQ entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment