[MGNLHOOK-307] Random problem reading environments on module startup Created: 15/Dec/22 Updated: 01/Feb/23 Resolved: 01/Feb/23 |
|
| Status: | Closed |
| Project: | Magnolia Webhooks |
| Component/s: | None |
| Affects Version/s: | 2.0.0 |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Javier Benito | Assignee: | Milan Divilek |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Σ Remaining Estimate: | Not Specified | Remaining Estimate: | Not Specified |
| Σ Time Spent: | 3h | Time Spent: | Not Specified |
| Σ Original Estimate: | Not Specified | Original Estimate: | Not Specified |
| Issue Links: |
|
|||||||||||||||||||||||||
| Sub-Tasks: |
|
|||||||||||||||||||||||||
| Team: | ||||||||||||||||||||||||||
| Date of First Response: | ||||||||||||||||||||||||||
| Epic Link: | Webhook on SaaS | |||||||||||||||||||||||||
| Sprint: | DevX 28, DevX 29 | |||||||||||||||||||||||||
| Story Points: | 5 | |||||||||||||||||||||||||
| Description |
| Comments |
| Comment by Milan Divilek [ 20/Jan/23 ] |
Tested SolutionsProblemI tested 3 possible solutions (Event via EventBus, Callback, subscriber lazyloading), but all have problem with "MgnlContext is not set" exception when info.magnolia.webhooks.registry.WebhookDefinitionEventHandler#createSubscriberFor creates info.magnolia.webhooks.events.core.EventSubscriber because it's running in different thread where MgnlContext is not set. "environment definitions loaded" callbackUnfortunately info.magnolia.config.source.ConfigurationSource doesn't support any callback and {EnvironmentBoundConfigurationSource}} is build via info.magnolia.config.plugin.S3ConfigurationSourcePlugin. This means for using callback we would need to do several API changes in info.magnolia.config.source.ConfigurationSource , info.magnolia.config.source.ConfigurationSourcePlugin, etc. Even it doesn't seems right to have defined callback for Environment related stuff in interfaces which doesn't know nothing about Environments. Using EventBus and firing event when loading environment is complete.Firing EnvironmentConfigurationLoadedEvent when environment is loaded here in #whenComplete clause. Disadvantage of this solution is that we are trying to minimalisite using of EventBus. Init subscribers when first webhook event is fired (lazyloading)Init info.magnolia.webhooks.events.core.EventSubscriber first time when info.magnolia.webhooks.events.core.EventSubscribers is called. This suffers also from the "MgnlContext is not set" problem, because webhook events are also triggered in different thread. Main issueMain issue here is in CLOUD-742, where we changed starting Environments to async. This may cause that that the environment/definitions are not ready when module starts. All the tested solutions above fixes the webhook module, but in case other modules needs defintions to be available at the start up, there's going to be same issue(which is hard to find). |
| Comment by Christopher Zimmermann [ 23/Jan/23 ] |
|
Regarding the "Main Issue" above: Is there something special about Webhooks that makes this issue more relevant for them? Or put another waay - is there a reason this issue has been reported against webhooks vs another definition type - such as restEndpoints? Is there another definition type that "needs definitions to be available at the startup"? or conversely - Why does webhooks need the definitions available at startup? Concretely, which other definitions might still cause problems? All of them? For example:
|
| Comment by Christopher Zimmermann [ 30/Jan/23 ] |
|
Thanks. OK. Well my 2cts would be that webhooks needing to fire during system startup is an edgecase and I would prefer not to support it at all if it would make the system more stable or easier to maintain. |
| Comment by Javier Benito [ 30/Jan/23 ] |
|
I'd say the current solution is ok, is working now. |