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 NPE in ScriptService when script file with no extension is deleted #7953

Merged

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Oct 1, 2014

Fixes #7689

listener.onFileCreated(file);
}
} catch (Throwable t) {
logger.warn("");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe log something here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems dangerous? Shouldn't this be similar to the other try/catches below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I missed it somehow. Fixed.

@s1monw
Copy link
Contributor

s1monw commented Oct 1, 2014

left one comment - other than that LGTM - shoud this go into 1.3 too?

@rjernst
Copy link
Member

rjernst commented Oct 1, 2014

The NPE fix, and test look good to me. But do we really need these try/catches around listeners? Aren't all the listeners impl details of our system? I'm afraid we will just silently ignore bugs in them then, instead of actually catching them.

@imotov imotov added the v1.3.5 label Oct 2, 2014
@imotov
Copy link
Contributor Author

imotov commented Oct 2, 2014

@rjernst because these listeners are called on a separate scheduled thread, catching and logging these failures earlier on the stacktrace allows as to isolate the failure easier. The tricky part about #7689 was that it was caused by an uncaught exception within listener in ScriptService. This exception wasn't mentioned anywhere in the ticket but it essentially messed up logic of the resource watcher and caused it to fail on every check afterwards with the error that was reported in the bug. So, I think this change (besides improving resource checker resiliency) will actually help us to find bugs easier.

I have updated the ticket with missed logging statement and improved test a bit to include a check that ScriptService is actually functional.

If v1.3.5 will ever happen I would love to see this fix there. This bug is kind of annoying.

@s1monw
Copy link
Contributor

s1monw commented Oct 2, 2014

LGTM

@s1monw s1monw removed the review label Oct 2, 2014
@imotov imotov force-pushed the issue-7689-npe-on-file-without-ext-deletion branch from 812bd07 to 384114f Compare October 3, 2014 19:19
@imotov imotov merged commit 384114f into elastic:master Oct 3, 2014
@imotov imotov removed the v1.3.5 label Oct 3, 2014
@imotov
Copy link
Contributor Author

imotov commented Oct 3, 2014

I didn't push it into 1.3 branch because 1.3 branch is missing changes added #6896 that this fix is relying on.

@s1monw
Copy link
Contributor

s1monw commented Oct 3, 2014

no worries - thanks!

@clintongormley clintongormley changed the title Fix NPE in ScriptService when script file with no extension is deleted Scripting: Fix NPE in ScriptService when script file with no extension is deleted Nov 3, 2014
@clintongormley clintongormley added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Mar 19, 2015
@clintongormley clintongormley changed the title Scripting: Fix NPE in ScriptService when script file with no extension is deleted Fix NPE in ScriptService when script file with no extension is deleted Jun 8, 2015
@imotov imotov deleted the issue-7689-npe-on-file-without-ext-deletion branch May 1, 2020 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v1.4.0 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullPointerException on ResourceWatcherService
4 participants