Skip to content

Conversation

@ascopes
Copy link
Owner

@ascopes ascopes commented Dec 4, 2022

This set of changes rips out a bunch of internal components around dealing with file managements that were not cleaning up correctly.

Previously, I used GC hooks to detect this stuff. It worked to begin with but it is very flaky with regards to circular references preventing object cleanup.

The new solution will provide an explicit Workspace type that holds all of the file resources. This is initialised by the user in their test and provided to the compiler when calling JctCompiler#compile. This workspace type implements the Closeable interface, so it can be used with a try-with-resources instead, removing the need for any GC hooks anywhere.

An example of how a test would look under this system:

class SomeTest {
  @JavacCompilerTest
  void testTheResourceCompilesCorrectly(JctCompiler<?, ?> compiler) {
    try (var workspace = Workspace.newWorkspace()) {
      workspace
          .createSourcePathPackage()
          .createFile("com/example/HelloWorld.java").withContents("""
              package com.example;

              public class HelloWorld {
                public static void main(String[] args) {
                  System.out.println("Hello, World");
                }
              }
              """
          );

      var compilation = compiler
          .compile(workspace);

      assertThatCompilation(compilation)
          .isSuccessfulWithoutWarnings();

      assertThatCompilation(compilation)
          .classOutput().packages()
          .fileExists("com/example/HelloWorld.class")
          .isNotEmptyFile();
    }
  }
}

The directory system that should be used can be provided as an argument to the Workspace constructor:

var workspace = Workspace.newWorkspace(PathStrategy.RAM_DIRECTORIES);
// or
var workspace = Workspace.newWorkspace(PathStrategy.TEMP_DIRECTORIES);

The default if no arguments are provided is to use PathStrategy.RAM_DIRECTORIES.

Other changes that facilitate this change include:

  • TempDirectoryFactory is removed.
  • JctFileManagerBuilder is removed, functionality is now in JctCompilationFactory.
  • JctCompilationFactory is now internal and hidden by the module descriptor.
  • JctCompilationImpl is now internal and hidden by the module descriptor.
  • AbstractJctCompiler is now internal and hidden by the module descriptor.
  • JctFileManagerImpl is now internal and hidden by the module descriptor.
  • TempDirectory is now internal and hidden by the module descriptor.
  • RamDirectory is now internal and hidden by the module descriptor.
  • TempDirectory no longer supports closure hooks or the closeOnGc argument.
  • RamDirectory no longer supports closure hooks or the closeOnGc argument.
  • BasicPathWrapperImpl is now internal and hidden by the module descriptor.
  • GarbageDisposalUtils has been removed entirely.
  • The pathwrappers package no longer exists. Types are now in the workspaces package.
  • Reintroduced impl subpackages for internal details.
  • Closeable types, including Container and ContainerGroup-derived types, will now throw IOException in their signature. The workspace class is exempt from this, instead throwing UncheckedIOException to prevent polluting test cases with checked exception requirements.
  • byte[] getClassBinary(String binaryName) throws IOException; methods are now removed as they were not used anyway. They were a leftover artifact of the old class loader mechanism that was used before I switched to using URLClassLoader.

@ascopes ascopes added bug Something isn't working housekeeping enhancement Optimisations and internal improvements in the codebase. labels Dec 4, 2022
@ascopes ascopes added this to the 1.0.0 milestone Dec 4, 2022
@ascopes ascopes self-assigned this Dec 4, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2022

Codecov Report

Merging #175 (b1cc15e) into main (6550af5) will decrease coverage by 1.52%.
The diff coverage is 48.55%.

@@            Coverage Diff             @@
##             main     #175      +/-   ##
==========================================
- Coverage   60.50%   58.97%   -1.53%     
==========================================
  Files          66       67       +1     
  Lines        2504     2467      -37     
  Branches      227      227              
==========================================
- Hits         1515     1455      -60     
- Misses        903      924      +21     
- Partials       86       88       +2     
Impacted Files Coverage Δ
...s/jct/assertions/AbstractJavaFileObjectAssert.java 0.00% <ø> (ø)
...thub/ascopes/jct/assertions/CompilationAssert.java 35.08% <ø> (+3.34%) ⬆️
...a/io/github/ascopes/jct/compilers/JctCompiler.java 30.43% <ø> (-40.62%) ⬇️
...ascopes/jct/compilers/impl/JctCompilationImpl.java 89.36% <ø> (ø)
...containers/impl/AbstractPackageContainerGroup.java 46.51% <0.00%> (ø)
.../jct/containers/impl/ModuleContainerGroupImpl.java 19.69% <0.00%> (ø)
.../jct/containers/impl/OutputContainerGroupImpl.java 35.13% <ø> (ø)
...jct/containers/impl/PackageContainerGroupImpl.java 33.33% <ø> (ø)
...ners/impl/PackageContainerGroupUrlClassLoader.java 100.00% <ø> (ø)
...jct/containers/impl/PathWrappingContainerImpl.java 50.90% <ø> (ø)
... and 21 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Dec 4, 2022

Test Results

   537 files   -      55     537 suites   - 55   4s ⏱️ ±0s
8 810 tests  - 1 374  8 734 ✔️  - 1 374  76 💤 ±0  0 ±0 
9 030 runs   - 1 374  8 954 ✔️  - 1 374  76 💤 ±0  0 ±0 

Results for commit b1cc15e. ± Comparison against base commit 6550af5.

♻️ This comment has been updated with latest results.

@ascopes ascopes force-pushed the bugfix/174-gc-hooks branch from 47ceb9a to 1a0ac4a Compare December 4, 2022 12:10
@ascopes ascopes marked this pull request as ready for review December 4, 2022 15:07
@ascopes ascopes linked an issue Dec 4, 2022 that may be closed by this pull request
@ascopes ascopes merged commit 0683176 into main Dec 4, 2022
@ascopes ascopes deleted the bugfix/174-gc-hooks branch December 4, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement Optimisations and internal improvements in the codebase.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GC hooks for closing resources are not working

3 participants