Projects tigase _server server-core Issues #758
AMP Message repository in XMLRepository (#758)
wojciech.kapcia@tigase.net opened 8 years ago
Due Date
2017-02-16

Starting server with enabled AMP plugin (component seems unrelated) and unsupported repository results in (seemingly?) circular dependency:

sess-man (class: tigase.server.xmppsession.SessionManager) {
    amp (class: tigase.xmpp.impl.MessageAmp) {
        amp () {
            amp () {
                amp () {
                    amp () {
                        amp () {
                            amp () {
                                amp () {
                                    amp () {
                                        amp () {
                                            amp () {
                                                amp () {
                                                    amp () {
                                                        amp () {
                                                            amp () {
                                                                amp () {
                                                                    amp () {
                                                                        amp () {
                                                                            amp () {
                                                                                amp () {
                                                                                    amp () {
                                                                                        amp () {
                                                                                            amp () {
                                                                                                amp () {
                                                                                                    amp () {
                                                                                                        amp () {
                                                                                                            amp () {
                                                                                                                amp () {
                                                                                                                    amp () {}
                                                                                                                }
                                                                                                            }
                                                                                                        }
                                                                                                    }
                                                                                                }
                                                                                            }
                                                                                        }
                                                                                    }
                                                                                }
                                                                            }
                                                                        }
                                                                    }
                                                                }
                                                            }
                                                        }
                                                    }
                                                }
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }

What's more, it influences startup of other beans/components.

wojciech.kapcia@tigase.net commented 8 years ago

It's a result of following code in @tigase.xmpp.impl.MessageAmp@:

	@Inject
	private MsgRepositoryIfc   msg_repo         = null;
	@Inject(nullAllowed = true)
	private MsgRepositoryIfc   msg_repo         = null;

It works (and doesn't prevent remaining components from starting).

As not all functionality of AMP depends on repository we could/should differentiate making availability of conditions/actions based on availability of the repository?

Nevertheless - failing injection of a bean in other bean (injecting repo in AMP) should not prevent other beans from loading (HTTP API component in this case)

Andrzej Wójcik (Tigase) commented 8 years ago

As not all functionality of AMP depends on repository we could/should differentiate making availability of conditions/actions based on availability of the repository?

Really? But then offline messages will not work, so server will not be properly storing messages as it should. I think we need to at least warn about this situation or even fully unload AMP processor as it should be done (code for that is in place, just need to check while it didn't work properly in this case)

Nevertheless - failing injection of a bean in other bean (injecting repo in AMP) should not prevent other beans from loading (HTTP API component in this case)

Yes, simple unload of bean which fails to initialize is in place, but for some reason it failed in this case. I need to check it.

wojciech.kapcia@tigase.net commented 8 years ago

Andrzej Wójcik wrote:

As not all functionality of AMP depends on repository we could/should differentiate making availability of conditions/actions based on availability of the repository?

Really?

For example dropping message if resource doesn't match (but figuring out which combinations depends on repository could be headache prone).

Also https://xmpp.org/extensions/xep-0079.html#process-s2s-disco defines (s2s means sender-to-sender, sic!) feature discovery so unsupported elements simply wouldn't be included.

But then offline messages will not work, so server will not be properly storing messages as it should. I think we need to at least warn about this situation or even fully unload AMP processor as it should be done (code for that is in place, just need to check while it didn't work properly in this case).

unload and load default message plugin, but in case when offline messages are disable AMP plugin simply fallbacks to message plugin just as well so instead of unloading messageamp bean altogether falling back to processing by message plugin could make more sense?

Nevertheless - failing injection of a bean in other bean (injecting repo in AMP) should not prevent other beans from loading (HTTP API component in this case)

Yes, simple unload of bean which fails to initialize is in place, but for some reason it failed in this case. I need to check it.

OK.

Andrzej Wójcik (Tigase) commented 8 years ago

Wojciech Kapcia wrote:

Andrzej Wójcik wrote:

As not all functionality of AMP depends on repository we could/should differentiate making availability of conditions/actions based on availability of the repository?

Really?

For example dropping message if resource doesn't match (but figuring out which combinations depends on repository could be headache prone).

Also https://xmpp.org/extensions/xep-0079.html#process-s2s-disco defines (s2s means sender-to-sender, sic!) feature discovery so unsupported elements simply wouldn't be included.

OK, but we should not allow to use server with invalid configuration as it will lead to bad impression.

But then offline messages will not work, so server will not be properly storing messages as it should. I think we need to at least warn about this situation or even fully unload AMP processor as it should be done (code for that is in place, just need to check while it didn't work properly in this case).

unload and load default message plugin, but in case when offline messages are disable AMP plugin simply fallbacks to message plugin just as well so instead of unloading messageamp bean altogether falling back to processing by message plugin could make more sense?

I disagree, if server is misconfigured it is better to turn it off than have server which partially works or even works with different configuration.

Nevertheless - failing injection of a bean in other bean (injecting repo in AMP) should not prevent other beans from loading (HTTP API component in this case)

Yes, simple unload of bean which fails to initialize is in place, but for some reason it failed in this case. I need to check it.

OK.

wojciech.kapcia@tigase.net commented 8 years ago

Andrzej Wójcik wrote:

OK, but we should not allow to use server with invalid configuration as it will lead to bad impression.

Well, originally this was about implementing AMP Message repository in XMLRepository (default in the distribution package) so with that there shouldn't be a problem with invalid configuration.

I disagree, if server is misconfigured it is better to turn it off than have server which partially works or even works with different configuration.

OK, but having a clear message why it was shut down would be a nice addition.

Andrzej Wójcik (Tigase) commented 8 years ago

I was able to find a bug in Tigase Kernel which caused issue with proper Tigase XMPP Server startup. It was related to improper removal of instance of bean implementing RegistrarBean interface. As a result it created many instances of Kernel class as visible in issue description.

After fixing this issue MessageAmp processor is unloaded properly if matching implementation of MsgRepositoryIfc is not available, which results in usable Tigase XMPP Server without MessageAmp processor. I think that this is correct behaviour for dependency injection issues.

%wojtek I have few questions about this issue at this point:

  1. We still may allow usage of MessageAmp without instance of MsgRepositoryIfc if it is desired behaviour? Should we support it?

  2. Do we still need implementation of MsgRepositoryIfc for @XMLRepository@? or my fix of dependency injection will be enough?

wojciech.kapcia@tigase.net commented 8 years ago

Andrzej Wójcik wrote:

I was able to find a bug in Tigase Kernel which caused issue with proper Tigase XMPP Server startup. It was related to improper removal of instance of bean implementing RegistrarBean interface. As a result it created many instances of Kernel class as visible in issue description.

After fixing this issue MessageAmp processor is unloaded properly if matching implementation of MsgRepositoryIfc is not available, which results in usable Tigase XMPP Server without MessageAmp processor. I think that this is correct behaviour for dependency injection issues.

%wojtek I have few questions about this issue at this point:

We still may allow usage of MessageAmp without instance of MsgRepositoryIfc if it is desired behaviour? Should we support it?

I would say but %kobit should make a final decission after our above discussion. Tigase requires repository in one form or another (authentication for example) so this could be considered a border case. (I would say we can disallow it and I'm 75% towards that)

Do we still need implementation of MsgRepositoryIfc for @XMLRepository@? or my fix of dependency injection will be enough?

It seems enough, however I see some usefullness in having relatively wide support of repositories in XMLRepository (running an automatic tests of repository), however with the recent switch of MUC from UserRepo to separate repository (and PubSub and other using separate repositories from the start) it may not make that sense to have MsgRepositoryIfc support in XMLRepository (if we could support all repositories in XMLRepository then sure, we can have it for completeness, otherwise I would skip it)

Just one comment about the fix - it looks like there are still multiple attempts to initialise those beans (?!):

2017-02-15 17:26:59.178 [main]             Kernel.injectDependencies()        WARNING:  Can't inject dependency to bean msgRepository (class: class tigase.server.amp.db.MsgRepository$MsgRepositoryMDBean) unloading bean msgRepository
2017-02-15 17:26:59.183 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean store (class: class tigase.server.amp.action.Store), skipping injection of this bean
2017-02-15 17:26:59.267 [main]             Kernel.injectDependencies()        WARNING:  Can't inject dependency to bean msgBroadcastRepository (class: class tigase.server.amp.db.MsgBroadcastRepository$MsgBroadcastRepositoryBean) unloading bean msgBroadcastRepository
2017-02-15 17:26:59.275 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean store (class: class tigase.server.amp.action.Store), skipping injection of this bean
2017-02-15 17:26:59.280 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean store (class: class tigase.server.amp.action.Store), skipping injection of this bean
2017-02-15 17:26:59.285 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean store (class: class tigase.server.amp.action.Store), skipping injection of this bean
2017-02-15 17:26:59.290 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean store (class: class tigase.server.amp.action.Store), skipping injection of this bean
2017-02-15 17:26:59.295 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean store (class: class tigase.server.amp.action.Store), skipping injection of this bean
2017-02-15 17:26:59.300 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean store (class: class tigase.server.amp.action.Store), skipping injection of this bean
2017-02-15 17:27:00.504 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.524 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.554 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.584 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.612 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.644 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.678 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.709 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.725 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.743 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.771 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.789 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.808 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.836 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.868 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.898 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.927 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.951 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:00.984 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.009 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.043 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.061 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.079 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.097 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.124 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.139 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.156 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean
2017-02-15 17:27:01.194 [main]             Kernel.injectDependencies()        WARNING:  Could not initialize bean amp (class: class tigase.xmpp.impl.MessageAmp), skipping injection of this bean

Also - is there anywere a description of bean naming, especially with the following change being made:

parent.unregister(b.getBeanName() + "#KERNEL");
Andrzej Wójcik (Tigase) commented 8 years ago

Wojciech Kapcia wrote:

Andrzej Wójcik wrote:

I was able to find a bug in Tigase Kernel which caused issue with proper Tigase XMPP Server startup. It was related to improper removal of instance of bean implementing RegistrarBean interface. As a result it created many instances of Kernel class as visible in issue description.

After fixing this issue MessageAmp processor is unloaded properly if matching implementation of MsgRepositoryIfc is not available, which results in usable Tigase XMPP Server without MessageAmp processor. I think that this is correct behaviour for dependency injection issues.

%wojtek I have few questions about this issue at this point:

We still may allow usage of MessageAmp without instance of MsgRepositoryIfc if it is desired behaviour? Should we support it?

I would say but %kobit should make a final decission after our above discussion. Tigase requires repository in one form or another (authentication for example) so this could be considered a border case. (I would say we can disallow it and I'm 75% towards that)

Ok, let's wait for %kobit

Do we still need implementation of MsgRepositoryIfc for @XMLRepository@? or my fix of dependency injection will be enough?

It seems enough, however I see some usefullness in having relatively wide support of repositories in XMLRepository (running an automatic tests of repository), however with the recent switch of MUC from UserRepo to separate repository (and PubSub and other using separate repositories from the start) it may not make that sense to have MsgRepositoryIfc support in XMLRepository (if we could support all repositories in XMLRepository then sure, we can have it for completeness, otherwise I would skip it)

I think that most of our repositories (PubSub,MUC,MA,UA) are not compatible with XMLRepository and it is always possible to use DerbyDB for testing purposes, so I would also be in favour of skipping this part.

Just one comment about the fix - it looks like there are still multiple attempts to initialise those beans (?!):

[...]

Yes, you are correct and it works as designed. We need to retry to create bean if any new bean of same type was added to the same kernel in case if one bean was required by the other one.

Also - is there anywere a description of bean naming, especially with the following change being made:

[...] it looks that beans with #KERNEL have special meaning (and are actually kernels)?

Beans with #KERNEL in names are in fact kernels of bean which name is used in a prefix. They are used to create kernel trees/dependency trees and to have isolation between component modules.

wojciech.kapcia@tigase.net commented 8 years ago

Andrzej Wójcik wrote:

Wojciech Kapcia wrote:

I would say but %kobit should make a final decission after our above discussion. Tigase requires repository in one form or another (authentication for example) so this could be considered a border case. (I would say we can disallow it and I'm 75% towards that)

Ok, let's wait for %kobit

as per above…

Artur Hefczyc commented 8 years ago

Andrzej Wójcik wrote:

Wojciech Kapcia wrote:

Andrzej Wójcik wrote:

I was able to find a bug in Tigase Kernel which caused issue with proper Tigase XMPP Server startup. It was related to improper removal of instance of bean implementing RegistrarBean interface. As a result it created many instances of Kernel class as visible in issue description.

After fixing this issue MessageAmp processor is unloaded properly if matching implementation of MsgRepositoryIfc is not available, which results in usable Tigase XMPP Server without MessageAmp processor. I think that this is correct behaviour for dependency injection issues.

%wojtek I have few questions about this issue at this point:

We still may allow usage of MessageAmp without instance of MsgRepositoryIfc if it is desired behaviour? Should we support it?

I would say but %kobit should make a final decission after our above discussion. Tigase requires repository in one form or another (authentication for example) so this could be considered a border case. (I would say we can disallow it and I'm 75% towards that)

Ok, let's wait for %kobit

I am kind of hesitant about this. It might be useful in some cases. In generally, I see a use-case for the Tigase running diskless, DB-less with repositories in memory only. So it would run without any persistence. But this would be a special feature, on which we would have to work, test it extensively.

As for the AMP I am not in favor of letting MessageAmp work without a repository just like this. To allow for such use-case we would have to test it, know and document exactly which functionality is available and which is now and document when this might be useful. In other words such a use case should be result of an intentional configuration in the Tigase server.

To sum it up. I would prefer to disable it for now and come back to this when we/customers really need it.

wojciech.kapcia@tigase.net commented 8 years ago

Artur Hefczyc wrote:

As for the AMP I am not in favor of letting MessageAmp work without a repository just like this. To allow for such use-case we would have to test it, know and document exactly which functionality is available and which is now and document when this might be useful. In other words such a use case should be result of an intentional configuration in the Tigase server.

To sum it up. I would prefer to disable it for now and come back to this when we/customers really need it.

Given the above we can Reject further XMPRepository adjustments (and problem with loading beans has been fixed).

There remain a problem of default configuration, for example for the purpose of running a web-installer (related: #4880) but for that purpose (and that particular config-type) we could change default plugin beans from message-amp to basic message plugin (if needed, setup doesn't relay on message exchange and even if none of the message plugins are not loaded then stanzas will simply get forwarded). %andrzej.wojcik

issue 1 of 1
Type
Bug
Priority
Normal
Assignee
RedmineID
4897
Version
tigase-server-8.0.0
Spent time
11h 15m
Issue Votes (0)
Watchers (0)
Reference
tigase/_server/server-core#758
Please wait...
Page is in error, reload to recover