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 support for Serverless Messaging (XEP-174) #9

Closed
wants to merge 8 commits into from

Conversation

OnlyInAmerica
Copy link

Let's use this pull request as a clean slate for evaluating XEP-174 Support. This pull request represents a baseline working implementation of link local chat.

The main unanswered question is how to handle mDNS service name (e.g david@example) changes. Perhaps we can interpret those changes as a signing-off of the old service name and a signing-on of the new service name in order to further clean up the code.

Flowdalic and others added 5 commits July 21, 2014 16:39
LLService, an abstract class, handles registering a local link service and
serves as the primary point of interaction for client developers.
JmDNSService is an LLService implementation which broadcasts the local link service
via Multicast DNS using the JmDNS library.

Some notable new classes employed by LLService:

LLServiceDiscoveryManager, a child class of ServiceDiscoveryManager,
connects local link Presences discovered by LLService to the
EntityCapsManager. This is needed because traditional presence packets
are not exchanged in a local link configuration.

XMPPLLConnection, a child class of XMPPTCPConnection, handles the
incoming/outgoing connection distinction and automatically establishes
a new XMPP stream with each peer connection.
Move all smack-serverless classes to org.jivesoftware.smack.serverless packgage per
OSGi convention. This involves changing a few access modifiers within ConnectionConfiguration,
XMPPTCPConnection.PacketReader and XMPPTCPConnection.PacketWriter
@OnlyInAmerica
Copy link
Author

Moved all smack-serverless classes to org.jivesoftware.smack.serverless, changing a few access modifiers (some methods of PacketWriter, PacketReader, ConnectionConfiguration) to accommodate the smack-serverless classes no longer sharing a package with other components of the library.

Pardon the new PR, wanted to preserve the old history and your comments while providing a nice one-commit PR as the merge candidate.

@jadahl
Copy link

jadahl commented Jul 23, 2014

I can't remember much, but as far as I can see the service name changes is used only to change our own service name. Then again I can't see that the service name change listener is ever added so might be broken anyway. Could possibly be related to service name collision detection or something like that. That might be the reason for invalidating a chat session, in case collision resolving changed the name. More or less only guessing from vague memories, but could be worth testing that at least.

@Flowdalic
Copy link
Owner

More or less only guessing from vague memories, but could be worth testing that at least.

While testing is always a good idea, I vote to remove the code we don't know the reason for its existence. There is nothing worse then code where nobody knows why it was written. After all it can be added later on if we (re-)discover the reason.

@jadahl
Copy link

jadahl commented Jul 24, 2014

Sounds reasonable to me.

@OnlyInAmerica
Copy link
Author

The only apparently valid case where a JmDNS service name would change is when a local client requests service registration with a service name conflicting with an existing client in the DNS cache. In this case, JmDNS will change the service name to achieve uniqueness. Here we have to notify the Smack components so we don't treat the requested service name (actually that of a pre-existing client) as ourself, and don't treat the actual service name (the unique name generated by JmDNS) as a foreign client.

However, this name change event occurs before the service is truly registered so it can't affect any Chats. It will be up to the application developer to handle persisted chat sessions under the old service name.

So in short the LLService#serviceNameChanged(newName, oldName) method will stay and be potentially invoked only on initial service registration via LLService#registerService(). Note this means we can remove the serviceNameChanged behavior from LLServiceDiscoveryManager because instances of that class aren't instantiated until a service is registered and an XMPPLLConnection has been made with a peer.

Other changes:
If I can modify LLChat to extend Chat:

  • Remove LLChatListener in favor of generic ChatManagerListener.
  • Remove LLConnectionListener in favor of generic ConnectionCreationListener.
  • Remove LLMessageListener in favor of generic MessageListener

If the local client requests a mDNS service name conflicting with one already
registered in the DNS cache JmDNS will alter the requested service name to avoid conflict before registration.
This change notifies LLService of the altered service name. This fixes issues around
treating the requested service name (actually the service name of a pre-existing peer)
as the local client, as well as treating the altered service name as a foreign client.

Remove logic that cleared the DNS cache when a conflict occurred. JmDNS chooses a
non-conflicting name before service registration so such action is not needed.
LLChat now extends Chat.
LLService has ChatManager handle incoming chats.

Removed LLChatListener in favor of generic ChatManagerListener.
Removed LLConnectionListener in favor of generic ConnectionListener.
Removed LLMessageListener in favor of generic MessageListener.
@Flowdalic Flowdalic force-pushed the master branch 4 times, most recently from dfd0ca3 to 393c713 Compare August 28, 2014 13:25
@Flowdalic Flowdalic force-pushed the master branch 13 times, most recently from 494d16f to af9b9a8 Compare September 5, 2014 13:58
@Flowdalic
Copy link
Owner

The Stream Management and Stream Feature branches have been merged. They include the last API changes that are relevant to smack-serverless, which needs now rebasing on the current master branch.

Sorry it took so long, but Stream Management isn't trival and was a top priority to Smack 4.1. If we can now smack-serverless in a mergeable state, then there is no reason for ChatSecure to use their own patched version of aSmack.

Please reply back if you are still interested in taking care of smack-serverless. There is no need to hurry, I just need to know if you are still with us. And it would be great if we could get smack-serverless done in Q4 2014.

@OnlyInAmerica
Copy link
Author

Absolutely want to finish this off. I'll get started. Will be in touch!
On Sep 11, 2014 3:04 PM, "Florian Schmaus" [email protected] wrote:

The Stream Management and Stream Feature branches have been merged. They
include the last API changes that are relevant to smack-serverless, which
needs now rebasing on the current master branch.

Sorry it took so long, but Stream Management isn't trival and was a top
priority to Smack 4.1. If we can now smack-serverless in a mergeable state,
then there is no reason for ChatSecure to use their own patched version of
aSmack.

Please reply back if you are still interested in taking care of
smack-serverless. There is no need to hurry, I just need to know if you are
still with us. And it would be great if we could get smack-serverless done
in Q4 2014.


Reply to this email directly or view it on GitHub
#9 (comment).

@Flowdalic
Copy link
Owner

For the record, the tracking issue is SMACK-262. The resulting single commit of this branch should contain this issue key.

@OnlyInAmerica
Copy link
Author

Will do. Expect to have time this week to settle this.

@Flowdalic
Copy link
Owner

Wasn't my intention to poke you. :) I believe this will need a few iterations before we have found the right API design and removed the duplicate code so that it's ready for inclusion. Glad to have you working on this.

@Flowdalic
Copy link
Owner

@OnlyInAmerica I've created a serverless branch based on your work. If you find the time, it would be great to see you in #smack to discuss further advancement. Thank you.

@Flowdalic
Copy link
Owner

I'm going to close this PR in favor of igniterealtime#25

After reviewing the state of the serverless messaging code I found that it has significant drawbacks and needs major rework.

  • XMPPLLConnection should behave like a standard XMPP connection. But in Add support for Serverless Messaging (XEP-174) #9 it's like a XMPP stream between two xep174 entities.
  • Instead LLStream should be introduced, which represents a direct connection between two xep174 entities
  • XMPPLLConnection holds a set of incoming/outgoing LLStream's
  • LLServer should hold references to XMPPLLConnections
  • The question of the bidirectionality of LLStreams needs to be cleared, maybe we need to add a 'useStreamsBidrectional' flag to XMPPLLConnection (defaults to false)
  • LLServiceDiscovery is unecessary and should get replace by LLEntityCapsManager (already done in do not lookup field types in FormFieldRegistry without a FORM_TYPE #25)
  • Something like a singleton LLRoster needs to be created, all XMPPLLConnections on a system see the same roster entries, namely the other announced xep174 presences.
  • smack-serverless should make use of java.nio

Work on that was already started in #25, but it's still a long way. I'm not sure if smack-serverless will make it into Smack 4.1, I may have to schedule it for a later release. smack-serverless is mentioned as job in https://github.com/igniterealtime/Smack/wiki/Smack-Jobs#create-xmppllconnection-for-serverless-link-local-messaging-xep-174.

I'd like to thank everyone working on smack-serverless so far, especially @OnlyInAmerica and @jadahl .

@Flowdalic Flowdalic closed this Dec 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants