[WIP] Fix leaking file handlers

[cmelchior]

Closes #165

Note: This PR is still work in progress.

This PR removes the close method from ClassPath as it is largely not needed. The only place where it was needed JarClassPath now handles it internally.
The implementation of JarClassPath is taken from: #165 (comment)

TODO:

• How do you correctly run unit tests? There seem to be multiple ways of doing it?
• ClassPath.openClassFile() has a weird contract. It both states that null can be returned if no class is found, but it can also throw NotFoundException. It is unclear when one is chosen over the other or if it is even the correct behaviour?
• Fix remaining unit tests

[nickl-]

How do you correctly run unit tests? There seem to be multiple ways of doing it?

Yes Unit tests $#%#$%# I fixed those issues and upgraded to JUnit 4 in PR #157
Tried again with subsequent patches to roll against master but it was so frustrating to test and since I've already done the work in #157 I wasn't going to waste any more time on tests.

All subsequent patches have been submitted based on the nickl-/javassist:junit-migrate branch as per #157 with PRs noting that they are dependent on #157

In addition to properly cleaning test resources and allowing test isolation it cleans up some warnings and includes hamcrest or complete JUnit4 freedom as per from this commit nickl-/javassist@4952c80

ClassPath.openClassFile() has a weird contract. It both states that null can be returned if no class is found, but it can also throw NotFoundException. It is unclear when one is chosen over the other or if it is even the correct behaviour?

Hmmm yes dodgy!!! Personally I prefer returning nulls but perhaps that is not everyone's choice... but it should be either or I agree with you.

Fix remaining tests

The current tests only seemed to test for ClassPath urls, I made some rudimentary checks to ensure that JarClassPath worked as expected. We probably need to include more tests for coverage with this patch.

[nickl-]

Review:

I see a lot of this patch is now blemished with fixes in order to be able to run tests which is counter productive. We should get #157 applied as a matter of urgency.

[nickl-]

Oh was there a Readme for tests... I don't like this!

You shouldn't have to look for documentation to be able to run tests they should simply work by running:

mvn test

[nickl-]

Ahh sweet #157 has been applied whoop whoop!!!!

I suggest apply the ClassPath changes to fresh HEAD and force push this branch to reflect the new changes.

[nickl-]

Question about the title? Please clarify what is meant by the use of "Windows" in the title.

[cmelchior]

Sorry, bad title. The effect is only felt on Windows, but the leak is happening on all platforms.

[nickl-]

Thought so... I could also pick it up on macOS... eventually.

Did you come right with the tests now from fresh HEAD?

[cmelchior]

Yes, the latest master have been merged.

A couple of questions:

• It looks like mvn surefire:test no longer work, so I assume mvn test is the way to run tests?
• Should I change the ClassPath.openClassFile() interface so it no longer throws NotFoundException or just note in Javadoc that null should be used instead of the exception? As changing the implementation might be a breaking change.

[cmelchior]

Also, with mvn package I'm seeing this test failure:

testJIRA150(javassist.JvstTest4)  Time elapsed: 0.92 sec  <<< FAILURE!
junit.framework.AssertionFailedError: performance test (the next try may succeed): 540 < 6 * 81
	at junit.framework.Assert.fail(Assert.java:57)
	at junit.framework.Assert.assertTrue(Assert.java:22)
	at junit.framework.TestCase.assertTrue(TestCase.java:192)
	at javassist.JvstTest4.testJIRA150(JvstTest4.java:693)

I'm not 100% if this indicate a bug in the new implementation of the benchmark is wrong somehow?

[chibash]

testJIRA150 is a performance test written long time ago.
Nowadays if your machine has a lot of? memory (> 8 or 16GB?) , it seems to fail.
I plan to remove it in near future.