Skip to content
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 id to iq ping stanza #93

Merged
merged 2 commits into from
Dec 29, 2015
Merged

Add id to iq ping stanza #93

merged 2 commits into from
Dec 29, 2015

Conversation

pixelrebel
Copy link
Contributor

Increments a new id for each ping. Currently, no id is provided so the response from the server is:
[Tue Oct 06 2015 12:18:47 GMT-0700 (PDT)] ERROR [xmpp error]<iq type="error" xmlns:stream="http://etherx.jabber.org/streams"><error type="modify"><bad-request xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/><text xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" xml:lang="en">Missing 'id'</text></error></iq>

Increments a new id for each ping.  Currently, no id is provided so the response from the server is:
`[Tue Oct 06 2015 12:18:47 GMT-0700 (PDT)] ERROR [xmpp error]<iq type="error" xmlns:stream="http://etherx.jabber.org/streams"><error type="modify"><bad-request xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/><text xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" xml:lang="en">Missing 'id'</text></error></iq>`
@anupdhml
Copy link
Contributor

anupdhml commented Oct 7, 2015

I think it's safe to have a static id, something like hubot_ping. IDs when sent from hubot are supposed to be ignored under XMPP, as we discussed in #63 (comment).

@markstory markstory added this to the 0.1.19 milestone Oct 7, 2015
@markstory
Copy link
Contributor

@anupdhml An increment avoids goofy servers grumping about duplicate ids. I am not sure this is a real concern though.

@pixelrebel
Copy link
Contributor Author

I used an increment because I saw the id incrementing in this example. However, slack was just as happy with a static id. Is there a concern for integer overflow using this increment method?

Frankly, after reading the xmpp spec, I'm starting to think this PR may not be the best idea. It's best to strictly adhere to the specs, right? Feel free to close this if you share the same concern.

@markstory
Copy link
Contributor

Is there a concern for integer overflow using this increment method?

No, Javascript uses 64bit floats for numbers. Which gives millions of years at one ping per second.

@shumphrey shumphrey mentioned this pull request Nov 25, 2015
@sonnyp
Copy link
Member

sonnyp commented Dec 24, 2015

@pixelrebel what's wrong about this PR regarding the spec?

BTW the spec was has a newer revision http://xmpp.org/rfcs/rfc6120.html

@pixelrebel
Copy link
Contributor Author

@sonnyp Nothing, I suppose. Looks like id is a required attribute for iq stanzas. Shall we move forward with this PR?

Oh and thanks for the version update. A nice christmas present!

@markstory
Copy link
Contributor

There are some merge conflicts. Can you fix them @pixelrebel? If not I can do it before merging.

@@ -108,7 +109,7 @@ class XmppBot extends Adapter
@reconnectTryCount = 0

ping: =>
ping = new ltx.Element('iq', type: 'get')
ping = new ltx.Element('iq', type: 'get', id: @currentIqId++)
Copy link
Member

Choose a reason for hiding this comment

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

new Stanza('iq', ...

markstory added a commit that referenced this pull request Dec 29, 2015
@markstory markstory merged commit 99136d8 into xmppjs:master Dec 29, 2015
@sonnyp
Copy link
Member

sonnyp commented Dec 29, 2015

cool, thanks @pixelrebel

@markstory
Copy link
Contributor

0.2.1 tagged and published to npm.

@pixelrebel
Copy link
Contributor Author

Thanks gents!

@pixelrebel pixelrebel deleted the patch-1 branch December 29, 2015 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants