Skip to content

New stream management option: ack_timeout#1287

Merged
prefiks merged 1 commit intoprocessone:masterfrom
weiss:ack-timeout
Sep 8, 2016
Merged

New stream management option: ack_timeout#1287
prefiks merged 1 commit intoprocessone:masterfrom
weiss:ack-timeout

Conversation

@weiss
Copy link
Member

@weiss weiss commented Sep 7, 2016

Close the connection if a stream management client fails to respond to an acknowledgement request within 60 seconds. This number of seconds can be changed with the new ack_timeout option, and the mechanism can be disabled by specifying infinity.

As a side effect of this change, a new acknowledgement is no longer requested before the response to the previous request is received.

Close the connection if a stream management client fails to respond to
an acknowledgement request within 60 seconds.  This number of seconds
can be changed with the new "ack_timeout" option, and the mechanism can
be disabled by specifying 'infinity'.

As a side effect of this change, a new acknowledgement is no longer
requested before the response to the previous request is received.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 32.013% when pulling 621f0e2 on weiss:ack-timeout into 7a538bb on processone:master.

@mremond mremond added this to the ejabberd 16.09 milestone Sep 8, 2016
?DEBUG("Timeout waiting for stream management acknowledgement of ~s",
[jid:to_string(StateData#state.jid)]),
close(self()),
fsm_next_state(StateName, StateData);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you set mgmt_ack_timer to undefined here, won't it be still set in case that session is resumed later?

Copy link
Member Author

@weiss weiss Sep 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's necessary, as the connection that resumes the session has a new fresh #state and mgmt_ack_timer is not inherited from the old state. But we could of course set mgmt_ack_timer = undefined to clarify the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we could of course set mgmt_ack_timer = undefined to clarify the code.

I was going to update the PR accordingly, but there's the minor downside that a new timer would be spawned if another stanza arrives before the process actually goes into the wait_for_resume state. This is avoided by not setting mgmt_ack_timer = undefined. We could set it to expired or something, though :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, i just was not sure how new session is created from old one, if this will not affect it, then i am ok with it.

@prefiks
Copy link
Member

prefiks commented Sep 8, 2016

Looks ok to me, so let's merge it.

@prefiks prefiks merged commit 6c943aa into processone:master Sep 8, 2016
@weiss weiss deleted the ack-timeout branch September 13, 2016 13:15
@lock
Copy link

lock bot commented Jun 9, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants