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

Merged
merged 1 commit into from Aug 16, 2016

Conversation

Projects
None yet
3 participants
@camlow325
Contributor

camlow325 commented Aug 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 11, 2016

Member

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)?

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 the JRuby 1.7.x label Aug 11, 2016

@kares kares added this to the JRuby 1.7.26 milestone Aug 11, 2016

@camlow325

This comment has been minimized.

Show comment
Hide comment
@camlow325

camlow325 Aug 11, 2016

Contributor

@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!

Contributor

camlow325 commented Aug 11, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@cprice404

cprice404 Aug 12, 2016

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!

Contributor

cprice404 commented Aug 12, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 12, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@camlow325

camlow325 Aug 15, 2016

Contributor

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!

Contributor

camlow325 commented Aug 15, 2016

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

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment