Avoid collecting stacktrace when building an OgnlException [Fixes OGNL-261] by davoustp · Pull Request #138 · orphan-oss/ognl

davoustp · 2021-11-18T10:37:12Z

No description provided.

lukaszlenart · 2021-11-18T11:02:04Z

@harawata @quaff @JCgH4164838Gh792C124B5 could you take a look on this PR?
I opt to merge this, here you can find more details.

harawata · 2021-11-18T19:10:45Z

Hello @davoustp @lukaszlenart !

Does this mean we lose some information that can be useful when investigating an issue?
Could you post a test code so that I can verify the difference before and after the change?

lukaszlenart · 2021-11-19T06:04:45Z

src/main/java/ognl/OgnlException.java

-                _initCause.invoke(this, new Object[] { reason });
-            } catch (Exception t) { /** ignore */ }
-        }
+        super( msg , reason, true, false);


Shouldn't this be controllable from outside?

Well, that's was the point of my question about the way to control this behavior change using whatever configuration mechanism is available.
Sorry that I was not clear enough. :-/

Personally, I would add a java prop and initialize a static boolean in OgnlContext (mimicking what's already done) to allow the user to control whether the stacktraces are collected when throwing OgnlExceptions or not.
This would allow to cope for unexpected situations when the previous behavior is actually required.

However, since OgnlContext is not already provided in any of the OgnlExceptionconstructor methods, this would stay a JVM-wide setting (meaning you would not be able to control this per OGNL evaluation).
Adding the OgnlContext and changing all code paths throwing this exception looks overkill to me.

Thoughts?

I meant that you can decide when creating the exception to collect or not the stack trace. I would just add another constructor and expose these true, falses and that's all :)

Then we can extend your idea and use OgnlContext to control if the exception should be created with stack trace or note (delegate the control out of exception itself).

Ah! Indeed, we can add the signature from java.lang.Exception.Exception(String, Throwable, boolean, boolean)
No big deal, I'll do it right away.

However, I'll leave this new constructor protected as this is intended to be used by sub-classes and not by code path throwing the exception - see java.lang.Throwable.Throwable(String, Throwable, boolean, boolean).
Makes sense?

Sure, makes sense 👍

Done and pushed.

davoustp · 2021-11-19T08:08:56Z

Hi @harawata

Does this mean we lose some information that can be useful when investigating an issue?

It depends.

If a cause exception is provided when building the OgnlException, then no, you won't loose any info since this cause exception is very likely to have a stacktrace collected (as this is the default for any Throwable), so you'll have the full code path that leads to the cause exception.

When raised by OnglRuntime in the case described by OGNL-261 , then yes, you don't have the stack trace anymore.
That's a consequence of using exceptions in nominal code path to signal a non-error condition (in this case, property could not be found while evaluating a compound statement).

If this is a problem, then that's why I asked if a configuration switch would be required (on or off by default, whatever you think is best - either maintain the previous behavior or go for the new behavior by default).

Could you post a test code so that I can verify the difference before and after the change?

You can use ognl.TestOgnlException.test_Throwable_Reason() and add a System.out.println(e.toString()); statement to see how that goes when a cause exception is provided.

There is currently no test around OgnlExceptionwhen no cause is provided.

davoustp · 2021-11-19T08:25:46Z

BTW, I'm wondering about the two ognl.OgnlException.printStackTrace(PrintStream) and ognl.OgnlException.printStackTrace(PrintWriter) methods:
the super.printStackTrace(s); invocation already prints the stack trace with the cause and suppressed exception stacktraces if any already.
Any reason to keep these methods (this currently actually prints the cause exception twice) ?

Added protected method to allow control on exception suppression and stacktrace collections to sub-classes.

davoustp · 2021-11-19T09:02:04Z

Any reason to keep these methods (this currently actually prints the cause exception twice) ?

Any objection against removal of these ognl.OgnlException.printStackTrace(PrintStream) and ognl.OgnlException.printStackTrace(PrintWriter) methods?

lukaszlenart · 2021-11-19T09:17:48Z

Any objection against removal of these ognl.OgnlException.printStackTrace(PrintStream) and ognl.OgnlException.printStackTrace(PrintWriter) methods?

I think it's ok to remove them

Remove printStackTrace overrides as this is now handled by base class already. Fixed typo in protected constructor's javadoc

davoustp · 2021-11-19T09:26:56Z

I think it's ok to remove them

Done.

lukaszlenart · 2021-11-19T09:42:27Z

I will release this changes as 3.4.x and also officially announce switching to Java 8 (I thought I already did that)

harawata · 2021-11-19T22:03:34Z

Thank you for the explanation @davoustp ,
I may not fully understand the impact of this change, but it seems to be the case that the current implementation has a problem, so I have no objection.

JCgH4164838Gh792C124B5 · 2021-11-20T18:14:32Z

Hello @lukaszlenart , @davoustp , and @harawata .

Thanks for including me in the discussion. :)

Also, thanks for the detailed explanation behind the changes, @davoustp .

After reviewing the changes for the PR, it looks like maybe some of the old code logic might have been intended to handle pre-JDK 1.4 conditions, but I could be mistaken ?

The the only potential risk that I could see with the printStackTrace() custom overridden method removals is that OGNL would no longer always print the stacktrace of the (non-null) cause/reason exception, in addition to the current exception stacktrace. If someone is relying on that extra information for diagnostics, then I think that they would have to modify their code to check if there is a non-null cause/reason, and print that stacktrace themselves when the OgnlException is caught. Probably an edge case, but you never know ...

It looks like the changes preserve the exception reason/cause via the "normal" Exception constructor call sequence, so that information appears to be preserved.

Other than that, there are two additional comments related to the discussion above:

Should an issue be opened against this GitHub repository to "match" OGNL-261, since this repository is somewhat distinct from commons-ognl ?
Did @lukaszlenart mean to mention 3.3.x (instead of 3.4.x) for a new OGNL release to include this change ? If so, does that mean he might create a new ognl-3-2-x branch as well (to separate out future main branch changes) ?

davoustp · 2021-11-21T21:58:43Z

Also, thanks for the detailed explanation behind the changes, @davoustp .

You're welcome. The better a change is understood, the more accepted it gets, in my opinion. ;-)

After reviewing the changes for the PR, it looks like maybe some of the old code logic might have been intended to handle pre-JDK 1.4 conditions, but I could be mistaken ?

I suppose you refer to the initCause() invocation - if yes, then I'd agree with you, this is my take on this as well: this looks like pre-JDK 1.4 exception chaining support.
As mentioned in the Throwable javadoc :

An example of using this method on a legacy throwable type without other support for setting the cause...

About:

The the only potential risk that I could see with the printStackTrace() custom overridden method removals is that OGNL would no longer always print the stacktrace of the (non-null) cause/reason exception, in addition to the current exception stacktrace.

I'm not sure what you mean here.
From the JDK 11 source code for java.lang.Throwable.printStackTrace(PrintStreamOrWriter), it first prints the exception (basically invoking toString()), then prints the stacktrace of this exception if any, moves on to print stacktraces of suppressed exceptions if any, and finally prints the stacktrace of the cause Throwable if any.

I don't see any way to have this method to "skip" printing the cause Throwable's stacktrace (if it has one), even if the OgnlException has no stack trace...
Did I overlook something?

lukaszlenart · 2021-11-22T07:57:34Z

Should an issue be opened against this GitHub repository to "match" OGNL-261, since this repository is somewhat distinct from commons-ognl ?

Yeah, it would be good to address this split personality and just keep one OGNL repository - I was planning move some changes from this repo into commons-ognl but this looks like overwhelming task, also preparing a release under ASF Commons is more complicated than here. So I was thinking about closing commons-ognl and keep supporting future of OGNL only here.

Did @lukaszlenart mean to mention 3.3.x (instead of 3.4.x) for a new OGNL release to include this change ? If so, does that mean he might create a new ognl-3-2-x branch as well (to separate out future main branch changes) ?

Yes & no - I missed that I had already switched to Java 8 but didn't push a new release, a few days ago I have released a new 3.3.0 version with Java 8 requirement and some fix. This change can be included in 3.3.1 or as a breaking change it can be included in 3.4.x. And I don't want to create dedicated branches if not needed, it's very hard to keep them in sync.

JCgH4164838Gh792C124B5 · 2021-11-27T19:27:30Z

Hi @davoustp .

About:

The the only potential risk that I could see with the printStackTrace() custom overridden method removals is that OGNL would no longer always print the stacktrace of the (non-null) cause/reason exception, in addition to the current exception stacktrace.

I'm not sure what you mean here. From the JDK 11 source code for java.lang.Throwable.printStackTrace(PrintStreamOrWriter), it first prints the exception (basically invoking toString()), then prints the stacktrace of this exception if any, moves on to print stacktraces of suppressed exceptions if any, and finally prints the stacktrace of the cause Throwable if any.

I don't see any way to have this method to "skip" printing the cause Throwable's stacktrace (if it has one), even if the OgnlException has no stack trace... Did I overlook something?

Ooops ... I missed the fact that the standard Throwable.printStackTrace() should print out the stacktrace of a non-null cause as well. That same behaviour shows in the JDK 8 source code as well. Thanks for pointing that out. :)

Since your changes set the cause (reason) in the constructor, the same information should be available, I think. With that in mind, only if someone is parsing logs for the old delimiters (e.g. "/-- Encapsulated exception ------------\\"), does it seem there might be a (probably very) small chance for an impact.

Thanks for your work on this PR, @davoustp. After the discussion, it looks good to me. 👍

davoustp · 2021-11-27T20:02:52Z

@JCgH4164838Gh792C124B5 you're welcome!
Glad I could help. :-)

lukaszlenart · 2021-12-30T13:49:35Z

LGTM and released, thanks @davoustp @harawata and @JCgH4164838Gh792C124B5 for the review and discussion 💪

OGNL-261 Avoid collecting stacktrace when building an OgnlException

e46b294

lukaszlenart reviewed Nov 19, 2021

View reviewed changes

OGNL-261 Avoid collecting stacktrace when building an OgnlException

6ddf10a

Added protected method to allow control on exception suppression and stacktrace collections to sub-classes.

OGNL-261 Avoid collecting stacktrace when building an OgnlException

65776a0

Remove printStackTrace overrides as this is now handled by base class already. Fixed typo in protected constructor's javadoc

davoustp force-pushed the master branch from 53442d3 to 65776a0 Compare November 19, 2021 09:25

lukaszlenart merged commit 90c70fa into orphan-oss:master Dec 30, 2021

lukaszlenart mentioned this pull request Mar 5, 2022

[OGNL-102] Improves performance when null was returned #140

Closed

lukaszlenart mentioned this pull request Jul 26, 2023

OGNL-102 Improves performance when null was returned #204

Merged

Uh oh!

Conversation

davoustp commented Nov 18, 2021

Uh oh!

lukaszlenart commented Nov 18, 2021

Uh oh!

harawata commented Nov 18, 2021

Uh oh!

lukaszlenart Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

davoustp Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

lukaszlenart Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

davoustp Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

davoustp Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

lukaszlenart Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

davoustp Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

davoustp commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davoustp commented Nov 19, 2021

Uh oh!

davoustp commented Nov 19, 2021

Uh oh!

lukaszlenart commented Nov 19, 2021

Uh oh!

davoustp commented Nov 19, 2021

Uh oh!

lukaszlenart commented Nov 19, 2021

Uh oh!

harawata commented Nov 19, 2021

Uh oh!

JCgH4164838Gh792C124B5 commented Nov 20, 2021

Uh oh!

davoustp commented Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszlenart commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JCgH4164838Gh792C124B5 commented Nov 27, 2021

Uh oh!

davoustp commented Nov 27, 2021

Uh oh!

lukaszlenart commented Dec 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davoustp commented Nov 19, 2021 •

edited

Loading

davoustp commented Nov 21, 2021 •

edited

Loading

lukaszlenart commented Nov 22, 2021 •

edited

Loading