I've found this issue as a result of an email which I received from Wojciech about recent changes to fix an issue with subscribe presence being stored to offline storage even though it was delivered to the user.

I think that change of behavior of SessionManager in the case of execution of postProcess method may have a negative impact on already existing implementations. I would suggest to revert it and change following conditions in MessageAmp and OfflineMessages processors:

session == null || !messageProcessor.hasConnectionForMessageDelivery(session)

to:

session == null || (packet.getElemName() == Message.ELEM_NAME && !messageProcessor.hasConnectionForMessageDelivery(session))

as in case of negative presence priority for connection only <message/> stanzas should be save to offline storage.

I've prepared proper test cases to cover this issue as well as a test case for the issue for which fix caused this issue.

Andrzej Wójcik wrote:

I've found this issue as a result of an email which I received from Wojciech about recent changes to fix an issue with subscribe presence being stored to offline storage even though it was delivered to the user.

Yes, my change to SessionManager was a result of failing one of the TTS test cases (basically PubSub 5 test case was failing because... of an presence packet retrieved from the offline store from one of the previous tests; what's worse - the mentioned presence packet was both stored to offline store and delivered to the user in the Presence 7 test). I've analysed the issue and discovered there were a couple of issues at hand:

• the logic of postProcess() method was off resulting in storing the message to offline store even though it shouldn't;

• executing postProcess() method in the first place even though the packet was correctly processed by Processor and shouldn't go through PostProcessors (and as docs state: "Post-processing - If there is no processor for the stanza then the packet goes through all post-processors.")

So I decided to fix the SessionManager logic to work as intended.

I think that change of behavior of SessionManager in the case of execution of postProcess method may have a negative impact on already existing implementations. I would suggest to revert it and change following conditions in MessageAmp and OfflineMessages processors:

[...]

to:

[...]

as in case of negative presence priority for connection only <message/> stanzas should be save to offline storage.

I've prepared proper test cases to cover this issue as well as a test case for the issue for which fix caused this issue.

OK, but right now MessageAmp and OfflineMessages implementations are basically utilizing a bug as a feature.

We could fix the implementations of postProcess but then we would need to update all documentation to state what is the current processing policy (basically all PostProcessors will be executed no matter whether there was or ther wasn't Processor to handle the stanza).

%kobit - what do you think?

I'm not sure if it was a bug or not, but it was working and now it is not.

To be honest I do not see any point to have a method which is called only if none of the processors handled the particular packet. What would be a point for having such a method if in this case default packet handler is called few lines later?

Also, the general idea for processors is that we need processors only if they need to do something and in another case we just forward packet to the destination.

I guess this is the question to Artur…

Also - it was also working just fine in 7.1.x so I guess logic that handles offline messages changed somewhere in 7.2.x line.

Wojciech Kapcia wrote:

I guess this is the question to Artur…

Ok, but... what is the question?

Also - it was also working just fine in 7.1.x so I guess logic that handles offline messages changed somewhere in 7.2.x line.

I am not sure if I correctly follow the discussion. Unfortunately it's been a long time since I looked in the source code, so I do not remember and do not understand all the logic details and implementation details.

The implementation should follow the spec (RFC or XEP). If the spec is not specific enough, then use common sense, look at this from the end-user perspective from the service perspective, from the performance perspective, etc....

I am even OK, to modify some code specifically for testing reasons, as long as this is the last criterion and does not affect above.

Artur Hefczyc wrote:

Wojciech Kapcia wrote:

I guess this is the question to Artur…

Ok, but... what is the question?

Whether:

• we should make code flow/logic follow what's defined in API ( postProcess() : Performs processing of packet for which there was no processor.)

OR

• adjust the API description so the packet will be processed by both XMPPProcessor and then also always by Postprocessor despite being already processed -- current offline messages handling make the use of it.

I guess if it works now and there wasn't anyone complaining about it before we can drop the requirement of "not being processed by processor" in case of PostProcessors (and reverse my recent "fix").

It is kind of confusing for me.

The way I remember designing and using it was to have postprocessing for packets which are not handled by any processor.

And the use case for this was storing messages into offline storage for users who are not online.

From what you describe it looks like it is now the other way around.

So, my answer is, we should think of uses cases for either of the logic and implement it the way that helps with the use-cases. I am not up to date enough with current code and implementation to make sensible decision.

On the other hand, if post processing is always executed, regardless whether processing was done, what's the point behind the postprocessing at all? It is kind of processing then.

Artur Hefczyc wrote:

It is kind of confusing for me.

The way I remember designing and using it was to have postprocessing for packets which are not handled by any processor.

That's what the API documentation say and what my fix does/brings back (not sure why it wasn't working like that before).

And the use case for this was storing messages into offline storage for users who are not online.

From what you describe it looks like it is now the other way around.

So, my answer is, we should think of uses cases for either of the logic and implement it the way that helps with the use-cases. I am not up to date enough with current code and implementation to make sensible decision.

On the other hand, if post processing is always executed, regardless whether processing was done, what's the point behind the postprocessing at all? It is kind of processing then.

I don't know, but the above kinda contradicts the below.

Andrzej Wójcik wrote:

I'm not sure if it was a bug or not, but it was working and now it is not.

To be honest I do not see any point to have a method which is called only if none of the processors handled the particular packet. What would be a point for having such a method if in this case default packet handler is called few lines later?

Also, the general idea for processors is that we need processors only if they need to do something and in another case we just forward packet to the destination.

%andrzej.wojcik - a bit silly question but - if those should be the same (from your point of view) then why put the code handling offline messages in post-processor? Or was it only like that because it was done this way in the past?

Let me stress it again - I only adjusted the code to be in line with what was intended/described - if the packet was processed then it should not go through postprocessing. However - looking at this particular code of SM - the code responsible for this two step distinction (process/postProcess) was like that for a long time...

But I think we are due to rethink how plugins/processors work (AFAIR there was already idea of a way to "chain" execution of the plugins, or better yet - define dependencies between plugins/processors -- it may be more feasible with Kernel in place?)

a bit silly question but - if those should be the same (from your point of view) then why put the code handling offline messages in post-processor? Or was it only like that because it was done this way in the past?

%wojtek

To be honest, offline processing code was always in postProcess() method, so I did not have seen any motivation to move anywhere else. After I added checks for connection priority, it was still working fine, so it was left as it was.

Some time ago it could be forced usage of postProcess() method due to how AMP processed messages, but it was changed already some time ago and now it may not be needed.

If we move this code to process() method then we would not have any class implementing PostProcessor interface - it will be dead.

While doing this we should consider unification as MessageAmp and OfflineMessages @postProcess()@ methods are very similar and in some parts are duplicating each other.

Also, it would be very good, if we could create separate processor of offline messages so storage of offline messages would be possible to be done on the separate thread pool. Placing them on the same processing pool as AMP and message delivery might not be a good idea.

Either way, we should fix this ASAP, as we are releasing SNAPSHOT builds which may drop offline messages.

Wojtek, +1 for changes in processing to some kind of chaining or ordering of processors, but the question is performance.

Andrzej Wójcik wrote:

Also, it would be very good, if we could create separate processor of offline messages so storage of offline messages would be possible to be done on the separate thread pool. Placing them on the same processing pool as AMP and message delivery might not be a good idea.

I think that was the initial idea behind having separate postProcessor - so as the name suggests OfflineMessages would only be called to store offline messages when neither AMP nor Message plugin were called.

And now I think I grasp the idea better - both AMP and Message plugin were processing based on session - if it's null then it would not deliver message and then you have postProcessor in OfflineMessages hence now if session is null (hence user being offline) it stores the message -- there was never enforcing of the skipping of processing postProcessor - it was merely a "contract" that no result was produced (or no processing took place).

Right now the offline message logic is itsy-bitsy more complicated and intertwined...

Either way, we should fix this ASAP, as we are releasing SNAPSHOT builds which may drop offline messages.

For now I've reverted my "fix".

Your proposal from #5692#note-2 should fix the problem at hand, but I'm not sure if it's ideal - as you suggested - we probably should revisit how offline messages are processed and handled. At any rate - I've went ahead and applied it.

OK, so reverting the change and applying Andrzej's fix to offline messages resolved both issues. However there is a problem at hand commented in #5692#note-13 (and later):

• how should offline messages be handled - there should be a separate, dedicated processor/postprocessor

• rethink how we define Plugins (Processors/PostProcessors) and what we expect from them.

%kobit %andrzej.wojcik %bmalkow ?

%wojtek I would suggest to leave it as it is for 7.2.0 so we could get closer to the release of 7.2.0 and modify logic in 7.3.0 by creation of new processor for offline messages.

I wasn't suggesting that we should deal with it in 7.2.0 - especially if we want bigger changes in Plugins.

I'm closing this issue as reported issue was fixed.