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

set cause on failed class load #265

Merged
merged 2 commits into from
Dec 10, 2023

Conversation

euclio
Copy link
Contributor

@euclio euclio commented Dec 8, 2023

If classloading fails due to an IOException, this patch sets the cause of the thrown ClassNotFoundException so that the root cause can be found.

This issue was discovered after my machine was throwing "too many open files" when attempting to open a JAR, but the class loader was hiding this cause.

Fixes #266.

@kriegaex
Copy link
Contributor

kriegaex commented Dec 9, 2023

The change looks reasonable, thank you. An integration test would be helpful. Can you tell me how to reproduce the problem? I could find out by myself, but do not have much time. Please provide a minimal reproducer. I can think about how to convert it into an IT myself, if our test structure is too confusing for you.

@euclio
Copy link
Contributor Author

euclio commented Dec 9, 2023

It's probably difficult to reproduce in a unit test environment. The issue happens when the class being loaded exists, but the loading fails with an IOException. In my case, it was because it was trying to open the JAR to load the class but I had hit the open file limit for my machine.

@kriegaex
Copy link
Contributor

I said integration test, not unit test. In a unit test, probably it would be easier to reproduce by stubbing a mock method. But thanks for the pointer in the right direction, I will think about it. Maybe a unit test is actually sufficient, maybe I come up with an idea for an IT.

@kriegaex
Copy link
Contributor

kriegaex commented Dec 10, 2023

@euclio, do you have one of your stack traces (after the change) for me? That would help to retrace the execution path below ExtensibleURLClassLoader::findClass.

I found out how to reproduce it in a unit test myself, but the stack trace would still be nice. Maybe you can edit the PR description and add it there for documentary purposes.

@kriegaex kriegaex self-assigned this Dec 10, 2023
@kriegaex kriegaex added this to the 1.9.21 milestone Dec 10, 2023
@kriegaex kriegaex added the enhancement New feature or request label Dec 10, 2023
@kriegaex kriegaex force-pushed the set-exception-cause branch from d79b309 to cbd199e Compare December 10, 2023 02:33
@kriegaex kriegaex merged commit 3ae6e98 into eclipse-aspectj:master Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set ClassNotFoundException root cause in ExtensibleURLClassLoader::findClass
2 participants