Wojciech Kapcia (Tigase) opened 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)
-
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
AMPprocessor 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.
-
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
AMPprocessor 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
messageplugin just as well so instead of unloading messageamp bean altogether falling back to processing bymessageplugin 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.
-
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
AMPprocessor 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
messageplugin just as well so instead of unloading messageamp bean altogether falling back to processing bymessageplugin 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.
-
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.
-
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
RegistrarBeaninterface. As a result it created many instances ofKernelclass as visible in issue description.After fixing this issue
MessageAmpprocessor is unloaded properly if matching implementation ofMsgRepositoryIfcis not available, which results in usable Tigase XMPP Server withoutMessageAmpprocessor. 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
MessageAmpwithout instance ofMsgRepositoryIfcif it is desired behaviour? Should we support it? -
Do we still need implementation of
MsgRepositoryIfcfor @XMLRepository@? or my fix of dependency injection will be enough?
-
-
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
RegistrarBeaninterface. As a result it created many instances ofKernelclass as visible in issue description.After fixing this issue
MessageAmpprocessor is unloaded properly if matching implementation ofMsgRepositoryIfcis not available, which results in usable Tigase XMPP Server withoutMessageAmpprocessor. 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
MessageAmpwithout instance ofMsgRepositoryIfcif 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
MsgRepositoryIfcfor @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 haveMsgRepositoryIfcsupport inXMLRepository(if we could support all repositories inXMLRepositorythen 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 beanAlso - is there anywere a description of bean naming, especially with the following change being made:
parent.unregister(b.getBeanName() + "#KERNEL"); -
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
RegistrarBeaninterface. As a result it created many instances ofKernelclass as visible in issue description.After fixing this issue
MessageAmpprocessor is unloaded properly if matching implementation ofMsgRepositoryIfcis not available, which results in usable Tigase XMPP Server withoutMessageAmpprocessor. 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
MessageAmpwithout instance ofMsgRepositoryIfcif 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
MsgRepositoryIfcfor @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 haveMsgRepositoryIfcsupport inXMLRepository(if we could support all repositories inXMLRepositorythen 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
XMLRepositoryand it is always possible to useDerbyDBfor 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
#KERNELhave special meaning (and are actually kernels)?Beans with
#KERNELin 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. -
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…
-
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
RegistrarBeaninterface. As a result it created many instances ofKernelclass as visible in issue description.After fixing this issue
MessageAmpprocessor is unloaded properly if matching implementation ofMsgRepositoryIfcis not available, which results in usable Tigase XMPP Server withoutMessageAmpprocessor. 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
MessageAmpwithout instance ofMsgRepositoryIfcif 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.
-
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
XMPRepositoryadjustments (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-ampto basicmessageplugin (if needed, setup doesn't relay on message exchange and even if none of themessageplugins are not loaded then stanzas will simply get forwarded). %andrzej.wojcik
| Type |
Bug
|
| Priority |
Normal
|
| Assignee | |
| RedmineID |
4897
|
| Version |
tigase-server-8.0.0
|
| Spent time |
0
|
Starting server with enabled AMP plugin (component seems unrelated) and unsupported repository results in (seemingly?) circular dependency:
What's more, it influences startup of other beans/components.