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

Formatter leaks threads and memory #846

Closed
stiemannkj1 opened this issue Oct 15, 2022 · 0 comments · Fixed by #890
Closed

Formatter leaks threads and memory #846

stiemannkj1 opened this issue Oct 15, 2022 · 0 comments · Fixed by #890

Comments

@stiemannkj1
Copy link
Contributor

stiemannkj1 commented Oct 15, 2022

The formatter fails to clean up the threads it creates which leaks the threads themselves (if the formatter is run multiple times) and the memory for the threads.

This also can prevent the JVM to from shutting down because the threads are non-daemon threads.

The reason this leak doesn't usually affect users is probably because users run the formatter via the CLI which calls System#exit to kill the JVM process.

Steps to Reproduce:

  1. Add this test to MainTest.java:

    private static Path createTestSourceFile(TemporaryFolder testFolder, String className)
        throws IOException {
      Path path = testFolder.newFile(className + ".java").toPath();
      Files.write(path, ("class " + className + " {}\n").getBytes(UTF_8));
      try {
        Files.setPosixFilePermissions(path, EnumSet.of(PosixFilePermission.OWNER_READ));
      } catch (UnsupportedOperationException e) {
        throw new IOException(e);
      }
      return path;
    }
    
    @Test
    public void doesNotLeakThreads() throws Exception {
      Path testA = createTestSourceFile(testFolder, "TestA");
      Path testB = createTestSourceFile(testFolder, "TestB");
    
      // Get the number of threads before formatting.
      int numberOfThreadsBeforeFormatting = Thread.currentThread().getThreadGroup().activeCount();
    
      // Run the formatter several times.
      for (int i = 0; i < 10000; i++) {
        final var errorCode =
            new GoogleJavaFormatToolProvider()
                .run(
                    new PrintWriter(
                        new BufferedWriter(new OutputStreamWriter(System.out, UTF_8)), true),
                    new PrintWriter(
                        new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true),
                    "--replace",
                    testA.toString(),
                    testB.toString());
        assertWithMessage("Error Code").that(errorCode).isEqualTo(0);
      }
    
      // Verify that the number of threads is nearly the same before and after. NB: we allow for a few
      // additional threads since we don't completely control the number of running threads in this
      // test.
      assertThat(Thread.currentThread().getThreadGroup().activeCount())
          .isLessThan(numberOfThreadsBeforeFormatting + 5);
    }
    
  2. Run the test:

     mvn test -Dtest="MainTest#doesNotLeakThreads"
    

If the test fails you'll get an OOM error:

[WARNING] Corrupted STDOUT by directly writing to native stream in forked JVM 1. See FAQ web page and the dump file /Volumes/Projects/google/google-java-format/core/target/surefire-reports/2022-10-15T16-48-29_971-jvmRun1.dumpstream
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.654 s <<< FAILURE! - in com.google.googlejavaformat.java.MainTest
[ERROR] doesNotLeakThreads(com.google.googlejavaformat.java.MainTest)  Time elapsed: 1.629 s  <<< ERROR!
java.lang.OutOfMemoryError: unable to create native thread: possibly out of memory or process/resource limits reached
        at com.google.googlejavaformat.java.MainTest.doesNotLeakThreads(MainTest.java:645)

java.lang.OutOfMemoryError: unable to create native thread: possibly out of memory or process/resource limits reached
        at java.base/java.lang.Thread.start0(Native Method)
        at java.base/java.lang.Thread.start(Thread.java:798)
        at java.base/java.util.concurrent.ThreadPoolExecutor.addWorker(ThreadPoolExecutor.java:937)
        at java.base/java.util.concurrent.ThreadPoolExecutor.ensurePrestart(ThreadPoolExecutor.java:1583)
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor.delayedExecute(ScheduledThreadPoolExecutor.java:346)
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor.schedule(ScheduledThreadPoolExecutor.java:562)
        at org.apache.maven.surefire.booter.ForkedBooter.launchLastDitchDaemonShutdownThread(ForkedBooter.java:369)
        at org.apache.maven.surefire.booter.ForkedBooter.acknowledgedExit(ForkedBooter.java:333)
        at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:145)
        at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
Exception in thread "main" java.lang.OutOfMemoryError: unable to create native thread: possibly out of memory or process/resource limits reached
        at java.base/java.lang.Thread.start0(Native Method)
        at java.base/java.lang.Thread.start(Thread.java:798)
        at java.base/java.util.concurrent.ThreadPoolExecutor.addWorker(ThreadPoolExecutor.java:937)
        at java.base/java.util.concurrent.ThreadPoolExecutor.ensurePrestart(ThreadPoolExecutor.java:1583)
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor.delayedExecute(ScheduledThreadPoolExecutor.java:346)
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor.schedule(ScheduledThreadPoolExecutor.java:562)
        at org.apache.maven.surefire.booter.ForkedBooter.launchLastDitchDaemonShutdownThread(ForkedBooter.java:369)
        at org.apache.maven.surefire.booter.ForkedBooter.exit(ForkedBooter.java:316)
        at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:425)

If the test passes the bug is fixed.

stiemannkj1 added a commit to stiemannkj1/google-java-format that referenced this issue Oct 15, 2022
stiemannkj1 added a commit to stiemannkj1/google-java-format that referenced this issue Oct 15, 2022
stiemannkj1 added a commit to stiemannkj1/google-java-format that referenced this issue Oct 16, 2022
stiemannkj1 added a commit to stiemannkj1/google-java-format that referenced this issue Oct 31, 2022
stiemannkj1 added a commit to stiemannkj1/google-java-format that referenced this issue Feb 3, 2023
stiemannkj1 added a commit to stiemannkj1/google-java-format that referenced this issue Feb 3, 2023
stiemannkj1 added a commit to stiemannkj1/google-java-format that referenced this issue Feb 3, 2023
copybara-service bot pushed a commit that referenced this issue Feb 4, 2023
I've signed the CLA.

Fixes #847

FUTURE_COPYBARA_INTEGRATE_REVIEW=#847 from stiemannkj1:fix-846-mem-thread-leak 0ca1e9b
PiperOrigin-RevId: 506946916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant