This patch could actually be improved for example by incrementing the messageWaiting counter only if the message has a body (we don't want an ack for chat states I think).

While I agree that it may be worth in some cases to send ack requests every time there is a message in out queue to reduce message redelivery but it may increase network traffic a lot which in case of mobile network will increase not only network usage but also will drain battery.

It will be better to change current implementation to allow to set custom number of messages awaiting ack which if bigger will trigger sending ack request, as forcing this to send always will be a waist o brandwith for stable connections.

What about creating an empty method in StreamManagementIOProcessor called something like shouldRequestAck() or something like that and let subclasses override that and return true or false to request ack (of course this would be OR'd with the current if condition).

This way we don't force users to use this behaviour but anyone could make their own.

By the way: is there a way to configure the XMPPIOProcessor class to be used?

I think that creation of this empty method shouldRequestAck() will be possible and it is good idea, but currently there is no possibility to specify XMPPIOProcessor classes to be used, so this would need to be added as well.

Andrzej Wójcik wrote:

I think that creation of this empty method shouldRequestAck() will be possible and it is good idea, but currently there is no possibility to specify XMPPIOProcessor classes to be used, so this would need to be added as well.

I see that the class involved is XMPPIOProcessorsFactory, but the PROCESSORS map contents are hard-coded. I could add a configuration parameter like this:

c2s/processorsClasses[s]=my.custom.class.name

Indexes of course will need to match the "cs2/processors[s]" parameter.

After reading this new parameter I can overwrite the values in the PROCESSORS map. What do you think?

In the meantime, I defined the shouldRequestAck part:

https://github.com/kontalk/tigase-server/commit/d384b503f325eb80c84e750fd9c2745b3b4c21f7

I'm sorry I realized now that my tigase-server repository is still private :)

I've attached the commit patch.

I splitted the feature in three commits:

https://github.com/kontalk/tigase-server/commit/8e02f4a3300b487558c121a180606e022decab70

Here I introduce IO processor class loading and configuration directive "processors-classes".

https://github.com/kontalk/tigase-server/commit/708ff727c1db6905f883f365fbc24315c83550a0

In this commit I moved OutQueue creation to a new overridable method newOutQueue (I know that's for my own purposes, but having an IO processor subclass with a customizable OutQueue can be very useful and might be the beginning for an interface to be defined in the future, but that's another story).

https://github.com/kontalk/tigase-server/commit/476f540c9974ca29bd4cfd67285e0636bb2fda42

In this commit I fixed a bug in the first commit.

I don't know if the class loading procedure is right (I mean as in Tigase-best-practices), if you review it and send me comments I'll be glad to improve it.

I added method shouldRequestAck() and possibility to override StreamManagementIOProcessor class which is implementation for id @urn:xmpp:sm:3@.

I added way to override XMPPIOProcessor implementations but in other way then your patch suggested. Until now it was possible to enable processors using following entry in etc/init.properties file:

c2s/processors[s]=urn:xmpp:sm:3

and now it is possible to enable XMPPIOProcessors and specify classes implementing it using following entry:

c2s/processors[s]=urn:xmpp:sm:3=tigase.server.xmppclient.StreamManagementIOProcessor

which forces class tigase.server.xmppclient.StreamManagementIOProcessor to be used as implementation for @urn:xmpp:sm:3@.

Previous way of configuration is still allowed and causes usage of default implementation for this id.

However I decided not to add method

protected OutQueue newOutQueue(XMPPIOService service)

Could you explain why you need XMPPIOService instance in metod responsible for creation of OutQueue? If so then maybe I could add other method which will better fit your needs and will not allow for possible memory leak.

Andrzej Wójcik wrote:

I added way to override XMPPIOProcessor implementations but in other way then your patch suggested. Until now it was possible to enable processors using following entry in etc/init.properties file:

which forces class tigase.server.xmppclient.StreamManagementIOProcessor to be used as implementation for @urn:xmpp:sm:3@.

Previous way of configuration is still allowed and causes usage of default implementation for this id.

Wonderful! Thank you.

However I decided not to add method [...] as described in https://github.com/kontalk/tigase-server/commit/708ff727c1db6905f883f365fbc24315c83550a0 as I do not like XMPPIOService instance being passed to newOutQueue method as it suggests that XMPPIOService instance may be passed somehow to OutQueue implementation and kept there which is wrong as it will create possibility to keep this instance even after stream resumption when old XMPPIOService should be destroyed. And in this case it would create possibility for potential memory leak.

Could you explain why you need XMPPIOService instance in metod responsible for creation of OutQueue? If so then maybe I could add other method which will better fit your needs and will not allow for possible memory leak.

That was implemented in a bit of hurry, I know... my actual need is to request an ack for every outgoing (as in sent to a client) , and I thought that an efficient way of telling if there is a message waiting for ack was to extend OutQueue and its behaviour.

I added methods to create instances of classes extending OutQueue and Counter which should be enough for what you need to do (but I created methods without any parameters as I think they are not needed)

Andrzej Wójcik wrote:

I added methods to create instances of classes extending OutQueue and Counter which should be enough for what you need to do (but I created methods without any parameters as I think they are not needed)

Wonderful, thank you! I'll adapt my code immediately and merge with yours.

I realized now that you assigned the issue to me: do I close it when I've verified everything is working?

Yes, please close this issue when you verify that changes works for you

Andrzej Wójcik wrote:

Yes, please close this issue when you verify that changes works for you

It does work, thank you. However I need to improve my implementation of shouldRequestAck: in some cases, using just a messageWaitingAck counter and just checking it for > 0 will request an ack for every packet sent after the first message until the client responds (e.g. offline messages delivered at login time). But that's my problem, thanks for your help on this.

Mmm.. how do I close this exactly? I think I'm lacking permissions.

Happy new year by the way!

OK, so I'm closing this task