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

Cleanup after tests to avoid OOME: Metaspace in fast-run testOnly sequence #254

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

retronym
Copy link
Member

@retronym retronym commented Jul 13, 2018

After these changes, this project no longer exhibits the problems
discussed in sbt/sbt#4166.

@typesafe-tools
Copy link

To validate this pull request against other sbt modules, please visit this link.

…uence

After these changes, this project no longer exhibits the problems
discussed in sbt/sbt#2056
@retronym retronym requested a review from eed3si9n July 13, 2018 05:32
@eed3si9n
Copy link
Member

I am guessing you mean sbt/sbt#4166.
I was using LM test as an example I had handy, but the OP likely had another build exhibiting the issue, so I think we should fix them either in ScalaTest or sbt.

@retronym
Copy link
Member Author

retronym commented Jul 14, 2018 via email

build.sbt Outdated
// shutdown Log4J to avoid classloader leaks
try {
val logManager = Class.forName("org.apache.logging.log4j.LogManager")
logManager.getMethod("shutdown").invoke(null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I am using async logging, I am guessing calling shutdown right away would risk some logs not showing up. But I think it would make sense to remove loggers that are no longer needed at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log4J might block until these are written:

    /**
     * Shutdown using the LoggerContext appropriate for the caller of this method.
     * This is equivalent to calling {@code LogManager.shutdown(false)}.
     *
     * This call is synchronous and will block until shut down is complete.
     * This may include flushing pending log events over network connections.
     *
     * @since 2.6
     */
    public static void shutdown() {
        shutdown(false);
    }

I haven't tested this.

I don't think it matters much for tests in any case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another problem is that some appenders are long-living (relay appender used by server, backing appender, I think), so I can't stop them. If the loggers are leaking the loaders then I'd need to find out how to prevent that in general, I think.

Copy link
Member Author

@retronym retronym Jul 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, this is shutting down Log4J that is in the classloader of the tests themselves, not Log4J used in the SBT build that is calling the tests.

If the test were forked, you get away without explicitly shutting down because Log4J registers a shutdown hook to clean up. But that shutdown hook becomes a classloader leak in a "container" environment like a webapp or SBT.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://logging.apache.org/log4j/2.x/manual/webapp.html

Because of the nature of class loaders within web applications, Log4j resources cannot be cleaned up through normal means. Log4j must be "started" when the web application deploys and "shut down" when the web application undeploys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Wouldn't val logManager = Class.forName("org.apache.logging.log4j.LogManager") load log4j in the context of the build tho?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right. I meant to use loader. I'll push a fix.

@eed3si9n
Copy link
Member

I locally still see metaspace climb to 250MB

metaspace2

@eed3si9n eed3si9n changed the base branch from 1.x to develop August 30, 2018 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants