Right now we are working on releasing version 7.1.0 and I think at this point we should move this patch to be included in 7.2.0.

As for patch, I think we need to refactor this patch as redelivery of message in case it connection was closed is not a good idea as it will cause issues with Message Archiving, AMP processing, etc. So we will need to adjust this redelivery part.

Also your queue algorithm has a flaw, when a lot of packets is sent to user in inactive state then queues can get very big and cause OOM - which is not good. In MobileV3 we flush queue when queue is too big, and when other packet is sent to client - ie. <iq/>@. We decided go this way as clients ofter relay on user presence during processing of some @<iq/> stanzas (jingle, etc.), and if you queue messages and presences while you deliver iq then you change order of XMPP stream which should not be done.

Another thing is that if client sends notification about inactive state for a second time without changing state to active then messages from queue will be lost!

Due to that I'm moving this to 7.2.0 as we need time to adjust this patch.

Thanks for the review! I will make the necessary fixes (those issues you pointed out need to be fixed anyway) and give you the result back.

I think we need to refactor this patch as redelivery of message in case it connection was closed is not a good idea as it will cause issues with Message Archiving, AMP processing, etc. So we will need to adjust this redelivery part.

Why do you think it will cause issues? I mean only messages are redelivered when stopped() is called, I'm not sure what you mean.

Redelivery will cause execution of every processors which should proceed packet - in this case message. This will cause MessageArchiving processor to be execute twice - so message will be stored 2 times (duplicate entry in DB).

I would also suggest to add <delay/> element to queued messages and presences as it may take a while before this packet will be delivered to client and if there is no <delay/> element then client assumes that timestamp is current time - which would not be true in case of delayed delivery.

Redelivery will cause execution of every processors which should proceed packet - in this case message. This will cause MessageArchiving processor to be execute twice - so message will be stored 2 times (duplicate entry in DB).

Oh I see: the AMP part is especially impacted by this.

How do you suggest to fix this? Doesn't MobileV3 do the same thing?

I would also suggest to add element to queued messages and presences as it may take a while before this packet will be delivered to client and if there is no element then client assumes that timestamp is current time - which would not be true in case of delayed delivery.

Totally right, thank you.

MobileV3 assumes queued packets as delivered and do not take care of redelivery. There is mechanism to deal with redelivery created for Stream Management support and I think it will need to be adjusted to make this work properly.

Thanks I understand now. Although, this approach is not strictly compatible with XEP-0352 - well, its business logic can be sort of... "intepreted", so I guess anything is possible.

Sadly, the XEP itself has been under last call since last year and have not been moved since. I'll try to resuscitate the relevant thread on the XSF mailing list.

One more question: if I filter a packet (that is, I remove it from the results queue before it can reach its recipient user), would it still be counted by Stream Management? If I don't use AMP and I only use OfflineMessages for storing messages, configured to not store what my CSI implementation is filtering, wouldn't I be ok (I mean besides the patch I'm giving you, I'm talking about my specific case)? (technically they are two questions, but they are related :)

Feature already implemented in #4244