[4.1.0-alpha1] SASL Authentication missing in OSGi

So as short term solution, I’m thinking about something like smack-java7-osgi, which depends on smack-extensions, smack-experimental and smack-java7 and provides a method SmackOsgi.activate(), that will have Smack’s OSGi bundles in active state when it returns. As far as I understand OSGi, this should be accomplished by calling the activate() methods of the respective SmackInitalizers of the Smack components. I’m not sure, but it appears to be that this would make Smack’s declarative services obsolete (correct me if I’m wrong).

I don’t know if I got you right, but in this case “smack-java7-osgi” must be a single jar containing all packages from “smack-java7”, “smack-core”, “smack-extensions”, “smack-experimental”, “smack-sasl-javax”, “smack-tcp” and “smack-resolver-javax”. The approach that you used to create “smack-java7” does not work in this case and the OSGi manifest file must export all API packages otherwise anybody is able to use the classes.

  • SaslProvider (offered by smack-sasl, required by smack-core)
    ATM the is no subproject smack-sasl, only smack-sasl-provided and smack-sasl-javax with the implementations for the platforms. Does this mean that there should be a smack-sasl subproject? Or shouldn’t SaslProvider go into smack-core?
  • SaslProvider (offered by smack-sasl, required by smack-core)
    ATM the is no subproject smack-sasl, only smack-sasl-provided and smack-sasl-javax with the implementations for the platforms. Does this mean that there should be a smack-sasl subproject? Or shouldn’t SaslProvider go into smack-core?

That was my fault, I meant: … offered by “smack-sasl-javax” and “smack-sasl-provided”, respectively.

The approach that you used to create “smack-java7” does not work in this case
Hmm, why? OSGi bundles transition into the activated state once a class they export are used. So we have smack-java7-osgi that has an Import-Package declaration containing all the packages form the Smack bundles smack-core, smack-extensions, smack-sasl-javax and so on. Then in smack-java7-osgi there is SmackOsgi.activate() which calls the activate() method from each of those Smack bundles. Once the method returns, Smack is fully activated.

Or am I missing someting?

and the OSGi manifest file must export all API packages otherwise anybody is able to use the classes.
“anybody is able to use the classes”? What’s wrong with that?

Flow schrieb:

The approach that you used to create “smack-java7” does not work in this case
Hmm, why? OSGi bundles transition into the activated state once a class they export are used. So we have smack-java7-osgi that has an Import-Package declaration containing all the packages form the Smack bundles smack-core, smack-extensions, smack-sasl-javax and so on. Then in smack-java7-osgi there is SmackOsgi.activate() which calls the activate() method from each of those Smack bundles. Once the method returns, Smack is fully activated.

Or am I missing someting?

See the problem from the consumer side: The problem is when my bundle imports packages e.g. from “smack-core” and my bundle gets activated, there is no gurantee (missing service layer) that “smack-java7-osgi” is activated before my bundle got activated, so the “activate” methods were not called. Package dependencies do not (unfortunately) influence activation order and I call e.g. new XMPPTCPConnection(…) but the SASLMechanisms were not added, because “smack-sasl-javax” gets activated later. If I would use the XmppConnectionBuilderFactory service from the OSGi registry instead, I would know that the bundle that provides the service has been activated before. The problem requires adequate service dependenices or a monolic bundle without any service where your init process will make sure that everything is setup. In this case we can rely on the init process that is triggered by classloading in Smack (e.g. AbstractXmppConnection) via this: “static { SmackConfiguration.getVersion(); }”

Ahh, I should have said that the short-term solution would involve telling OSGi users that they need to run manually SmackOsgi.activate() before they can use Smack.

If I would use the XmppConnectionBuilderFactory service from the OSGi registry instead, I would know that the bundle that provides the service has been activated before.
The problem here is that you easily build a dependency from smack-core to the optional smack subprojects/osgi bundles like smack-extensions. I think that is what makes the situation so complicated: We don’t want a dependency from smack-core to any optional subproject. But on the other hand, we want the optional subprojects to get initialized when they are available.

Sorry for the delay… I had to do some codings. I have created a fork of Smack: Herr-Herner/Smack · GitHub

I have started with an implementation of a service layer for Smack under OSGi, but it is incomplete and mainly untested… After some bugs, all services are now available in the OSGi registry and ready for usage. At the moment, I could not check, if the service based approach really works. Therefore, I have to update my sources. These are currently the main services (-> provides, <- requires):

  • smack-java7: PlatformToolkit ->
  • smack-resolver-javax: DNSResolverProvider ->
  • smack-sasl-javax: SASLMechanismsProvider ->
  • smack-core:
    • ProviderRegistry ->
    • XmppProvider ->
    • XmppProvider <- { PlatformToolkit, DNSResolverProvider, SASLMechanismsProvider }
  • smack-tcp:
    • XMPPTCPConnectionBuilderFactory ->
    • XMPPTCPConnectionBuilderFactory <- XmppProvider
  • smack-extensions:
    • ExtensionsProvider ->
    • PubSubManagerBuilderFactory ->
    • PubSubManagerBuilderFactory <- ExtensionsProvider
      In OSGi you must add a dependency to the services “XMPPTCPConnectionBuilderFactory” to build up a “XMPPTCPConnection” to the server and e.g. in my case “PubSubManagerBuilderFactory” for creating a “PubSubManager”. I think this approach guarantees that Smack is completely setup, when those services become available, but I must validate that.

Any thoughts?

Wow, ~800 new LOC.

Your effort is appreciated, but to be clear and I don’t want to discourage you, I’m not sure if this will make it into 4.1. I’ve to thoroughly review the code and give it some thoughts. The top priority for 4.1 is at the moment, now that xep198 is merged, smack-serverless, so that ChatSecure can switch to Smack 4.1. And those changes affect a lot of Smack internals. I could imagine that it would be hard for this “osgi” branch to track master.

Why is smack-android removed from settings.gradle? Or was this just done for prototyping?

Ok, I had a quick look at the code, and I think I do understand most of what’s going on there. But since I don’t work on a daily basis with OSGi some questions arise.

The biggest caveat right now is XMPPTCPConnectionBuilder, as I planned to apply the builder pattern at ConnectionConfiguration, to separate settings what require a new connection instance (like the service name) from those who can applied on the fly (like PacketReplyTimeout). The former settings are meant for ConnectionConfiguration, the later ones are just setter/getter methods in (Abstract)XMPPConnection (and their subclasses for settings that only apply to e.g. XMPPTCPConnection).

With your change XMPPTCPConnectionBuilder becomes what should be done with ConnectionConfiguration.

What purposes serves XMPPTCPConnectionBuilder? Is it just that smack-tcp is initialized? Couldn’t we do it e.g. with a newly created class called XMPPTCPConnectionConfiguration.Builder() instead?

Also of course, those changes should be applied to smack-bosh (and smack-serverless once it hit master) too.

Other then that, I really like it!

I think the XMPPConnectionBuilder interface should be like

<X extends XMPPConnection, C extends ConnectionConfiguration> X build(C connectionConfiguration)

so that we can apply the builder pattern to ConnectionConfiguration and use the XMPPConnectionBuilder for smack-tcp, smack-bosh and smack-serverless, e.g.

XMPPTCPConnection build(ConnectionConfiguration conf)

and

XMPPBOSHConnection build(BOSHConnectionConfiguration conf)

Flow schrieb:

Wow, ~800 new LOC.

Your effort is appreciated, but to be clear and I don’t want to discourage you, I’m not sure if this will make it into 4.1. I’ve to thoroughly review the code and give it some thoughts. The top priority for 4.1 is at the moment, now that xep198 is merged, smack-serverless, so that ChatSecure can switch to Smack 4.1. And those changes affect a lot of Smack internals. I could imagine that it would be hard for this “osgi” branch to track master.

Thanks… No problem, you don’t discourage me, but I am a little bit under time pressure here and try to move forward as fast as possible. My problem is that I am (unfortunately) not familar with Git’s branching capabilities. I am still using SVN and have not found the time to migrate to Git. It’s also the first time that I have created a fork… What is the right approach. Should we create an OSGi branch?

Why is smack-android removed from settings.gradle? Or was this just done for prototyping?

Just for prototyping… I had to remove it, because the build process fails otherwise with an unset Android Home. What must I do to get things build including smack-android. Is the Android SDK required?

Flow schrieb:

I think the XMPPConnectionBuilder interface should be like

<X extends XMPPConnection, C extends ConnectionConfiguration> X build(C connectionConfiguration)

so that we can apply the builder pattern to ConnectionConfiguration and use the XMPPConnectionBuilder for smack-tcp, smack-bosh and smack-serverless, e.g.

XMPPTCPConnection build(ConnectionConfiguration conf)

and

XMPPBOSHConnection build(BOSHConnectionConfiguration conf)

Ok, in this case I recommend to call it XMPPConnectionFactory with a single method <X extends XMPPConnection, C extends ConnectionConfiguration> X newConnection(C config). That’s nice, so I can rid of the *BuilderFactory classes. Do I understand it right: You create the corresponding connection instances (TCP, BOSH) via reflection in the XMPPConnectionFactory, right? Each ConnectionConfiguration offers access to the corresponding class object and each Connection class must have a corresponding constructor e.g. XMPPTCPConnection(TCPConnectionConfiguration config).

but I am a little bit under time pressure
Why? There exists a workaround (manually initializing Smack in a OSGi environment). Or am I missing something?

and have not found the time to migrate to Git

I definitely recommend using git. The branching capabilities that we will need here are just one of the strengths of this fine DVCS.

What is the right approach. Should we create an OSGi branch?
Yes, it would be common practice if you would create an osgi branch where you put your commits. The master branch of your repo yields usually the same contents as the “official” master branch.

I suggest we approach this as follow: You maintain the your osgi branch, where you can experiment with the changes. You don’t have to care about commit style, if your branch is ready for master inclusion, it will be squash merged. The typical workflow you would apply is: Commit osgi related changes into your osig branch, and once in a while, merge the changes from master into it (which may require conflict resolution).

You can ping me, once you are happy with the state of the osgi branch, then I’m going to review the changes and provide feedback.

Please be aware that this may take a while until such things are ready for master inclusion. For example the xep198 branch took 5 months and the smack-serverless PR is open for a few months now (Add support for Serverless Messaging (XEP-174) by OnlyInAmerica · Pull Request #9 · Flowdalic/Smack · GitHub ).

I have backed up my modifications, deleted the repo and forked Flowdalic/Smack. I have also created a branch named “osgi” and made the necessary modifications on ConnectionConfiguration for following the builder pattern. I have added a XMPPConnectionFactory… Is this what you had in mind? Unfortunatly there are issues in the code, I cannot fix because there are several code pieces with modifiy the ConnectionConfiguration after creation. I would rework the service layer, when this approach is OK.