I think this is possible only if the stanza was queued for delivery in XMPPIOService in the waiting list, but was not written to the socket yet, as in another case it would be in OutQueue and would be handled during disconnection by StreamManagementIOProcessor (which would actually resend message after socket is closed, ie. after resumption timeout, see https://github.com/tigase/tigase-server/blob/09d0b30e7a7cd8056e627469cbed176d932c47df/src/main/java/tigase/server/xmppclient/StreamManagementIOProcessor.java#L535 and https://github.com/tigase/tigase-server/blob/09d0b30e7a7cd8056e627469cbed176d932c47df/src/main/java/tigase/server/xmppclient/StreamManagementIOProcessor.java#L584). In both those cases SM is sending errors (scheduling redelivery) after calling ConnectionManager::serviceStopped(service) which actually close the connection.

On the other hand, a call to ConnectionManager::processUndeliveredPacket() in ConnectionManager::serviceStopped() is made before service/connection is closed, as this is done in subclass ClientConnectionManager::serviceStopped() after method in the parent class is called.

While I think we could move scheduling redelivery to be called after the connection is closed (and STREAM_CLOSED, and STREAM_FINISHED are sent). This would result in moving the call to this code from ConnectionManager to ClientConnectionManager (and maybe other subclasses) making it a non-standalone, which feels wrong.

However, currently, scheduling of unacked packets (from StreamManagement queue), is now done after the connection is closed and after packets from waiting queue are resent. As messages in OutQueue (SM queue) are older than messages from waiting queue, I think we should make sure that they are redelivered before messages from waiting queue. That would also result in a non-standalone ConnectionManager class.

I think that the best way would be to reimplement this part (redelivery) to take packets from OutQueue and the waiting queue and process them in a single run to make sure that the ordering of packets is correct. Moreover, I think, it might be desirable to think about merging OutQueue and waiting queue somehow as both contain sent (or unacked) packets, while waiting queue is missing timestamps of those packets (however, it should be processed fast, unless connection IO/TCP buffer will saturate).

I'm not sure if we can cleanly solve this issue without API changes and a rather large reimplementation. Maybe just minor changes would work, but the order of stored messages might be incorrect. I also have mixed feeling about relying on offline storage in a multiclient environment and would suggest the usage of MAM for message syncing and dropping the usage of offline storage).

@wojtek What do you think? How should we proceed?

While I think we could move scheduling redelivery to be called after the connection is closed (and STREAM_CLOSED, and STREAM_FINISHED are sent). This would result in moving the call to this code from ConnectionManager to ClientConnectionManager (and maybe other subclasses) making it a non-standalone, which feels wrong.

ClientConnectionManager in itself has somewhat different processing logic so it wouldn't be that "non-standard", though looking at the code, I agree making the change would require bigger change.

I think it's OK to make API changes considering that those would be only addition (and possible removals would be deprecated first). How much time do you estimate for the proposed single queue implementation?

I'm not sure if we can cleanly solve this issue without API changes and a rather large reimplementation. Maybe just minor changes would work, but the order of stored messages might be incorrect. I also have mixed feeling about relying on offline storage in a multiclient environment and would suggest the usage of MAM for message syncing and dropping the usage of offline storage).

I think this isn't necessarily multi-client approach. Judging form the payload it's most likely kontalk with it's very non-standard groupchat (i.e. doesn't use MUC but each client broadcast the same message to all participants, hence possible "bulk messages" and most likely higher CPU usage / slower queue processing). Yes, MAM may be a solution, but in the case of "very secure" deployments they most likely don't use MAM and operate on volatile messages.

How much time do you estimate for the proposed single queue implementation?

That is a difficult question. While I would prefer not to extend "waiting queue" as it is used frequently and may contain a lot of items (also for S2S and due to that may increase memory usage), this may be necessary to have a correct timestamp of messages in offline storage.

But.. we can try to "move" redelivery to the correct place and hope it will work fine without further changes. In the worst-case scenario, we would be forced to reimplement queuing mechanism. What do you think?

In Tigase 9.0, we should (as we discussed) unify MAM storage with offline storage. Then we may not need to redelivery those messages to offline storage, as they would be already there if not acked by the client (or not fully sent), so I would prefer not to spend too much time on this.

Yes, MAM may be a solution, but in the case of "very secure" deployments they most likely don't use MAM and operate on volatile messages.

Is storing messages in MAM a lot different from storing messages in offline storage? Basically, the only change is that after delivering the message, it "should" be removed. On the other hand, it forces single-client usage to ensure that your client has a full history of messages. I think that MAM with short "expiration" would work almost the same.

@wojtek I've applied discussed changes and they are for now stored in a separate branch. I've also added a test case to TTS-NG to verify this fix. Please review it when you will have some time and then we will merge it and release it.

I checked the changes (and created PR: https://github.com/tigase/tigase-server/pull/188#pullrequestreview-1304619926) - it looks good IMHO.

In Tigase 9.0, we should (as we discussed) unify MAM storage with offline storage. Then we may not need to redelivery those messages to offline storage, as they would be already there if not acked by the client (or not fully sent), so I would prefer not to spend too much time on this.

Agreed.

Yes, MAM may be a solution, but in the case of "very secure" deployments they most likely don't use MAM and operate on volatile messages.

Is storing messages in MAM a lot different from storing messages in offline storage? Basically, the only change is that after delivering the message, it "should" be removed. On the other hand, it forces single-client usage to ensure that your client has a full history of messages. I think that MAM with short "expiration" would work almost the same.

This is reasonable. And in case of "pure offline store" messages can be removed from MAM upon offline client connecting (just like it happens right now). But yes, having MAM by default for everything would simplify a lot of back-and-forth and redelivery. (feel free to create task for 9.0 to address the change)