search

sbabcoc / JUnit-Foundation

JUnit Foundation is a lightweight collection of JUnit watchers, interfaces, and static utility classes that supplement and augment the functionality provided by the JUnit API.
Apache License 2.0
22 stars 6 forks source link

drop dead code, capturing test arguments, causing memory leak #90

Closed sns-seb closed 3 years ago

sns-seb commented 3 years ago

Hello,

At SonarSource, we went and used JUnit-Foundation for a small project.

Using it on modules with a large number of tests, we could observe a memory leak which leads to OOM errors.

We got to the root of it and it turned out the leak was causes by static Maps holding references to every single test objects created, for ever.

Luckily, these Maps are either:

This pull request proposes to apply these changes.

Cheers,

sbabcoc commented 3 years ago

Thanks for the contribution. I'm currently away from my desk, but I'll take a closer look when I get back. None of the code is "dead", as these are public interfaces. I'll need to fix the out-of-memory failures in a more targeted fashion.

sbabcoc commented 3 years ago

The reason for holding onto the test class instances was to provide post-run access to instance variables like parameterized test arguments. I can easily dispose of these objects at the end of each test.

sns-seb commented 3 years ago

Hello!

I sure don't know the tool in details but what I can observe is that:

Anyway, it's just my two cents on some design considerations. Not very important.

As long as memory leak is fixed, I don't mind how :)

Feel free to ping me to test/validate your implementation.

Cheers!

sbabcoc commented 3 years ago

Thanks for the feedback! It's greatly appreciated. I'll be able to publish a new release later today that drops the accumulated test resources that are causing out-of-memory failures.

sbabcoc commented 3 years ago

This is going to take a bit longer than I thought to add code that releases test framework objects at strategic points. The functionality will remain unchanged, so you may be able to work with a SNAPSHOT built from your branch in the meantime.

sns-seb commented 3 years ago

Hello,

I've been working with a build of a fork of the project since before I created this PR. This allowed me to test the fix I shared ;)

Let me know when you're done with your fix, I'll happily drop my fork for a release in Maven Central.

Cheers,

sbabcoc commented 3 years ago

Check out pull request #91 I'll be publishing a new release with these revisions later today. Thank you so much for choosing my library and contributing to its success!

sbabcoc commented 3 years ago

JUnit Foundation 13.0.0 has been published. Let me know if this new release resolves your out-of-memory issue.

sbabcoc commented 3 years ago

@sns-seb Hope you've seen this.

sns-seb commented 3 years ago

Hello @sbabcoc,

I'm not working on the project which uses JUnit-Foundation at the moment. I hope I'll find time next week and let you know of the results of upgrading to 13.0.0.

Thank you,

Cheers

sbabcoc commented 3 years ago

Hi, @sns-seb ,

Let me know how it goes. You should see a significant reduction in memory consumption. There's now a 13.0.1 release out there, but this was primarily a code documentation update. I added JavaDoc to the settings, and I deprecated the RuleChainWalker static utility class. It's likely that no one besides me was actually using this class, so the impact is essentially zero.

Cheers!

sns-seb commented 3 years ago

Hello @sbabcoc,

I just tested today with 13.0.1 and got an OOM running the tests on my project (no OOM when running without JUnit-Foundation).

I'll try and find the time tomorrow to find out what's going on.

Cheers,

sbabcoc commented 3 years ago

Hi, @sns-seb,

Thanks for your continued efforts on this issue. I may be misinterpreting how ServiceLoader provider declaration work. Please try adding the following entries to the file at resources/META-INF/services/com.nordstrom.automation.junit.JUnitWatcher in your project hierarchy:

com.nordstrom.automation.junit.RunAnnouncer
com.nordstrom.automation.junit.ReferenceRemover

The second of these is the watcher that releases object references at key points. I don't expect that this will make any difference, but I also expected that release 13.0.1 would resolve your OOM issues.

Fingers crossed!

sbabcoc commented 3 years ago

Hi, @sns-seb,

In tracing through the execution, I discovered that the management of the CHILD_TO_CALLABLE collection is not releasing mappings for configuration methods. I'm working on a fix for this now, but it could take me a few days to turn this around. I also discovered that I'm not releasing mappings for notifier objects, which are collected in RUNNER_TO_NOTIFIER. I'll figure this one out, too. If you figure out which specific objects are accumulating and using up your memory, that would be extremely helpful.

Cheers!

sns-seb commented 3 years ago

Hello @sbabcoc ,

There is indeed a leak in CHILD_TO_CALLABLE collection which retains references to org.junit.runners.model.FrameworkMethod instances.

I debugged a small Unit Test and found that:

  1. ReferenceRemover is being invoked
  2. I can observe the following objects being used as key to CHILD_TO_CALLABLE
    • child = {JUnit4@3813} + runner = {JUnit4@3813}
    • child = {FrameworkMethod@4329} + runner = {JUnit4@3813}
  3. I can observe the following release calls
    • call to releaseCallablesOf({JUnit4@3813}) from ReferenceRemover
    • recursive call to releaseCallablesOf({FrameworkMethod@4329})
    • call to RunAnnouncer.getAtomicTestOf({FrameworkMethod@4329}) returns null => reference for "child = {FrameworkMethod@4329} + runner = {JUnit4@3813}" not removed
sns-seb commented 3 years ago

Hello @sbabcoc ,

The reason for holding onto the test class instances was to provide post-run access to instance variables like parameterized test arguments. I can easily dispose of these objects at the end of each test.

I just realized something: keeping track of all the data in CHILD_TO_CALLABLE and RUNNER_TO_NOTIFIER doesn't come for free. If not because of extra memory usage, it will be because of extra concurrency (and potential bottleneck). On huge test runs, it can make a difference. How about doing all this only when needed (eg. only when user implement a specific listener)? If it's too hard to detect relevant cases, it could explicitly be enabled by user when she needs it.

sbabcoc commented 3 years ago

Hi, @sns-seb,

Thanks for the insights. If I manage the objects correctly, I think these issues can be minimized. If no watchers are registered, I could potentially disable the collection of these objects. I'm in the process of figuring out what I did wrong in regard to collection of references and the composition of AtomicTest objects. There are definitely fundamental issues to sort out in the flow of events and collection of references.

I'll keep you updated on my progress.

Cheers!

sbabcoc commented 3 years ago

Hi, @sns-seb,

Still working on the set of issues you encountered. I have a PR in process (#93) that I hope to have finished up soon.

sbabcoc commented 3 years ago

Hi, @sns-seb,

Release 14.0.0 has been published. This contains major improvements in how I manage framework object references and should resolve the memory consumption issues you were experiencing with prior releases. I had to revise a few public interfaces in this release, which informed the major-version bump.

Thanks, and let me know how it goes!

sbabcoc commented 3 years ago

Hi, again, @sns-seb,

Try release 14.0.1 instead, as the prior release contains an event sequencing issue.

Cheers!

sns-seb commented 3 years ago

Hello @sbabcoc ,

I tested 14.0.1 and this time no memory leak to observe.

Thanks!

sbabcoc commented 3 years ago

Hi, @sns-seb !

That's great news! Thank you so much for your contributions to this library! I found more than a few additional issues due to the in-depth examination of the flow of execution required to fix the memory consumption issues.

That's the downside to being a one-man operation. No one else is checking my work to see where I screwed up! ;-)

I'm going to add more unit tests to ensure that I don't sneak any memory issues in with future revisions. I also need tests to verify that start/finish notifications are balanced.

Cheers!