Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIx nested InvocationTargetException #4192

Merged
merged 5 commits into from
Aug 15, 2023

Conversation

bentsherman
Copy link
Member

Close #3418

When a method invoked via reflection throws an exception, the exception is wrapped in an InvocationTargetException. BaseScript already has a catch block to unwrap this exception, but for some reason there are some cases where the exception is wrapped twice, such as the linked issue.

I tried burying the user exception in more function calls and workflow calls, but it was only ever wrapped once or twice. So it does not seem to depend on the call depth, but I can't explain why it is wrapped sometimes once and sometimes twice. So I just check both cases.

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@netlify
Copy link

netlify bot commented Aug 14, 2023

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit 6b656d3
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/64dba00c6cb61000085fc6ed

@bentsherman
Copy link
Member Author

A more general solution...

def err = e
while( err instanceof InvocationTargetException )
    err = err.cause

@pditommaso
Copy link
Member

pditommaso commented Aug 15, 2023

yeah, but it looks that it would make more sense

def err = e
while( err instanceof InvocationTargetException )
    err = err.target
Screenshot 2023-08-15 at 17 21 46

@bentsherman
Copy link
Member Author

bentsherman commented Aug 15, 2023

According to the docs, targetException is the same as cause, it's just older from before cause became the standard for all exceptions. That's why I updated the other occurrence in TaskProcessor.

@pditommaso
Copy link
Member

Not according to the reality .. oops, screenshot was broken.

@bentsherman
Copy link
Member Author

Looking at the screenshot, my guess is that getTargetException() and getCause() both map to target. The docs say they are identical, so I don't really care which one we use.

@pditommaso
Copy link
Member

Good point

    public Throwable getTargetException() {
        return target;
    }

    /**
     * Returns the cause of this exception (the thrown target exception,
     * which may be {@code null}).
     *
     * @return  the cause of this exception.
     * @since   1.4
     */
    @Override
    public Throwable getCause() {
        return target;
    }

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso pditommaso merged commit 67980f1 into master Aug 15, 2023
6 checks passed
@pditommaso pditommaso deleted the 3418-fix-nested-invocation-target-exception branch August 15, 2023 16:03
pditommaso added a commit that referenced this pull request Aug 23, 2023

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
abhi18av pushed a commit to abhi18av/nextflow that referenced this pull request Oct 28, 2023

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants