[MAGNOLIA-6445] Don't swallow exceptions in Op.exec() / doInSystemContext Created: 18/Nov/15  Updated: 04/Mar/19

Status: Open
Project: Magnolia
Component/s: core
Affects Version/s: 5.4.2
Fix Version/s: None

Type: Bug Priority: Major
Reporter: Magnolia International Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: system-context
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File Prevent_NPEs_on_GuiceUtil.patch    
Issue Links:
Relates
relates to MAGNOLIA-6363 Unclosed sessions on doInSystemContex... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

When an exception occurs in Op.exec() or even potentially in MgnlContext.getSystemContext(), AND MgnlContext.release() ALSO fails, the former exception is lost; neither logged nor thrown.

IMO, the former exception is the one we want to be throwing, and a further exception in release should be ignored (but logged anyway)

example

This happens a lot, btw, especially during development, since we start components/guice through a doInSystemContext call from MagnoliaServletContextListener; any misconfigured or missing component, wrong constructor, etc, just results in NPE in info.magnolia.objectfactory.guice.GuiceUtils#hasExplicitBindingFor because of this.
(FYI see attached patch; I believe if this error here gets fixed, the NPE won't happen again, however, having an explicit exception rather than an NPE would have helped as well).

Test added in MAGNOLIA-6363 is also questionable. It seems overly complicated and undocumented. At the very least, it should say what it does/why (i understand there's probably no easy way to test those pesky static methods).



 Comments   
Comment by Mikaël Geljić [ 18/Nov/15 ]

efochr I believe what Greg was telling us was not to specifically prevent NPEs on GuiceUtils, but rather make sure we don't swallow exceptions in the first place. Then that NPE should not occur anymore to start with.

Changing the type of exception thrown in GuiceUtils is just a side-issue that "would have helped", for the example (improving the symptoms).

Generated at Mon Feb 12 04:14:35 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.