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

Write a log message when findClass IOException is caught #4062

Conversation

camlow325
Copy link
Contributor

This commit just adds a simple LOG.debug message to output the contents of an IOException that might be caught by the JRubyClassLoader during a findClass call. We were encountering the IOException in cases where our process ran out of file descriptors, leading the classUrl.openStream call to fail. This manifested in our application as a NoClassDefFoundError downstream but because the IOException is swallowed higher up, it was initially hard for us to able to determine what the root cause was. Being able to see this new debug message in the log output would significantly help with our ability to diagnose this kind of problem.

@kares
Copy link
Member

kares commented Aug 11, 2016

e.message will be already printed since the whole exception object is being passed, do you need it twice, does the IOException happen a lot (just asking whether to confirm whether the stack-trace is useful)?

@kares kares added this to the JRuby 1.7.26 milestone Aug 11, 2016
@camlow325
Copy link
Contributor Author

@kares I've only seen this exception pop up when something bad happens when the jar file is being consumed, like the process is out of file descriptors. I expect that for many apps (like ours) that this exception may end up being fatal - since a needed class won't be found later on. So I wouldn't expect it too appear too frequently for one application run in the log output.

We found having the full exception stack trace in the log to be pretty helpful in determining where the call to findClass was coming from. You're right that as I have the PR currently that the exception message would appear in both the initial message line and the exception detail, using the standard error logger. I had just found it handy to see the message in both places for visibility but it's definitely not crucial to do it that way.

Do you see a downside to including the whole exception in the log - especially considering that it's only at debug level and debug isn't enabled by default? If we're okay with including the full exception, would you prefer that I take the e.getMessage() part out of the message string?

Thanks much for reviewing this!

@cprice404
Copy link
Contributor

@kares it'd be very helpful for us if this logging could be merged prior to the next 1.7.x release, which I believe @enebo said might be able to happen next week?

If so, we are happy to change this up however you see fit so that it can be merged. Thanks!

@kares
Copy link
Member

kares commented Aug 12, 2016

fine with me if the IOException is rare to occur. I only found @camlow325's other (related) bug/PR later.

@camlow325
Copy link
Contributor Author

I think it would be pretty rare for this exception and message to be written to the log. The only time I've seen it logged is for the case where the process runs out of file descriptors, which seems like it would be relatively rare. This is the exact case where having the extra detail would be really helpful.

@kares - are you okay with merging this PR as-is, then? Thanks again!

@kares kares merged commit 0dc6fa9 into jruby:jruby-1_7 Aug 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants