Scripting: Wrap groovy script exceptions in a serializable Exception object #6628
Conversation
| fail("should have thrown an exception"); | ||
| } catch (SearchPhaseExecutionException e) { | ||
| assertThat(ExceptionsHelper.detailedMessage(e) + "should have contained GroovyScriptExecutionException", | ||
| ExceptionsHelper.detailedMessage(e).contains("GroovyScriptExecutionException"), equalTo(true)); |
There was a problem hiding this comment.
It'd be nice to be sure it contained that not_found wasn't found.
There was a problem hiding this comment.
Good idea! I've added this.
|
@kimchy I've added logging and catching a more generic exception to this PR. I also discovered that compilation exceptions don't serialize properly either, so I've done the same thing if an exception occurs during compilation. |
| return loader.parseClass(script, generateScriptName()); | ||
| } catch (Exception e) { | ||
| if (logger.isTraceEnabled()) { | ||
| logger.trace("Exception compiling Groovy script:", e); |
There was a problem hiding this comment.
can we log the groovy script name here? and start with lower case e :) . Also, catch Throwable in case of assertions enabled?
There was a problem hiding this comment.
the script name is going to be "Script1.groovy", "Script2.groovy", "Script3.groovy", is that actually helpful?
There was a problem hiding this comment.
What do you think about catching AssertionError directly? I am worried that catching Throwable means catching OutOfMemoryError and swallowing the exception (which we have to do for the groovy exceptions).
|
@dakrone added a few more comments |
|
@kimchy thanks! I've addressed all of the feedback except for catching Throwable, instead I used |
| return loader.parseClass(script, generateScriptName()); | ||
| } catch (Exception|AssertionError e) { | ||
| if (logger.isTraceEnabled()) { | ||
| logger.trace("exception compiling Groovy script:", e); |
There was a problem hiding this comment.
I think you forgot to add the name of the script here to the failure log
|
LGTM, lets do the script name (since its tricky) in another change ++ |
|
Opened #6653 for this. |
clintongormley
changed the title
clintongormley
added
the
:Core/Infra/Scripting
Fixes #6598
It prevents ES from trying to serialize the default Groovy exceptions, which want to carry over a lot of state that doesn't serialize properly.