OutOfMemoryError in cycle check

[crehn]

I'm using ArchUnit in three systems now. Two smaller systems (around 5kLOC) and a larger one (125kLOC, 1000 classes, 100 packages). In every system there is a cycle check:

@ArchTest
public static final ArchRule shouldHaveNoPackageCycles = slices().matching("com.acme.system.(**)")
        .should().beFreeOfCycles();

In the smaller systems this check is fast (around 200ms) and works perfectly fine. But in the larger system the test crashes after a minute throwing an OutOfMemoryError while using up more than 3GB of RAM.

java.lang.OutOfMemoryError: Java heap space
	at java.util.Arrays.copyOf(Arrays.java:3332)
	at java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:124)
	at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:448)
	at java.lang.StringBuilder.append(StringBuilder.java:136)
	at java.lang.StringBuilder.append(StringBuilder.java:76)
	at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:484)
	at java.lang.StringBuilder.append(StringBuilder.java:166)
	at java.lang.StringBuilder.append(StringBuilder.java:76)
	at com.tngtech.archunit.thirdparty.com.google.common.base.Joiner.appendTo(Joiner.java:109)
	at com.tngtech.archunit.thirdparty.com.google.common.base.Joiner.appendTo(Joiner.java:154)
	at com.tngtech.archunit.thirdparty.com.google.common.base.Joiner.join(Joiner.java:197)
	at com.tngtech.archunit.thirdparty.com.google.common.base.Joiner.join(Joiner.java:187)
	at com.tngtech.archunit.lang.ConfiguredMessageFormat.formatFailure(ConfiguredMessageFormat.java:31)
	at com.tngtech.archunit.lang.FailureReport.toString(FailureReport.java:62)
	at com.tngtech.archunit.lang.ArchRule$Assertions.assertNoViolation(ArchRule.java:92)
	at com.tngtech.archunit.lang.ArchRule$Assertions.check(ArchRule.java:82)
	at com.tngtech.archunit.lang.ArchRule$Factory$SimpleArchRule.check(ArchRule.java:196)
	at com.tngtech.archunit.library.dependencies.SliceRule.check(SliceRule.java:64)
	at com.tngtech.archunit.junit.ArchRuleExecution.evaluateOn(ArchRuleExecution.java:41)
	at com.tngtech.archunit.junit.ArchUnitRunner.runChild(ArchUnitRunner.java:154)
	at com.tngtech.archunit.junit.ArchUnitRunner.runChild(ArchUnitRunner.java:63)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at com.tngtech.archunit.junit.ArchUnitRunner$1.evaluate(ArchUnitRunner.java:88)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:252)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:141)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:112)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

Checking for cycles in sub-packages works fine, though:

@ArchTest
public static final ArchRule shouldHaveNoPackageCycles = slices().matching("com.acme.system.layer1.(**)")
        .should().beFreeOfCycles();

for each of the four layers. This is my current workaround: for each layer I have a separate cycle test and a layeredArchitecture() ensures that there are no cycles between layers.

After examining the stack trace I guess that my workaround is leaky and there is some weird large cycle that produces a violation description output of several GB -- maybe because there is some unnoticed infinite recursion in some description generation. But that guess might also be completely off. I don't know and I don't know enough about the inner workings of ArchUnit, yet.

[codecholeric]

The cycle detection can unfortunately start to use up a big amount of heap simply for the violations, I've noticed that, too. I've used it once in a big project and had to introduce a really ugly hack to cut off all details after the first line. The problem is AFAIK, that ArchUnit lists all dependencies of every edge of every cycle found.
And that can unfortunately become really large. Imagine you have a cycle with 20 edges and 19 are legal and have 20000 dependencies. Then one edge with one dependency closes the cycle. ArchUnit will print all dependencies of every edge. Also ArchUnit prints all the cycles found, so often it is not just one big cycle, but also many additional small cycles sharing a lot of the edges. So in the end ArchUnit runs out of memory simply because there are so many lines of text involved to describe the violations.
I've not really had the right idea to tackle this, maybe a configuration after how many "details" ArchUnit should cut off the message? Because in many other cases it's also nice to see all dependencies involved in the violation message.
As for your concrete problem, you should maybe also slice bigger and check

@ArchTest
public static final ArchRule shouldHaveNoPackageCycles = 
    slices().matching("com.acme.system.(*)..")
        .should().beFreeOfCycles();

I.e. group your classes by the top-level package under system and check those for cycles. You should at least get cycles that are not so long in this case. When you've solved those cycles, maybe your original test would not die anymore?
I'm pretty sure that the test dying means you have some evil cycles 😉

[crehn]

Thanks at lot! That additional test helped me to track down some dependency cycle in my tests.

As for this test and all the others I'd really appreciate a more concise output. Not only for cycle checks but in general.

Something like that:

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'foo' was violated (17 times):
Class ..control.Foo depends on class ..boundary.Bar (12 times)
Class ..control.EarthWorm depends on class ..boundary.Squirrel (5 times)

So:

• a common package prefix like "com.acme.department.system" is removed
• packages like java.lang and maybe java.util could also be removed from the output by default
• dependencies are grouped by classes and the number of dependencies is shown to get a feeling of how tight the coupling is
• if I need more information I could add .debug() at the end of the rule and rerun the test. This would produce the full output like today.

[codecholeric]

Hmm, I see your point, I know the output is very verbose. Unfortunately I think this would be a very big thing to implement at the source, i.e. where the error messages are created. Also in parts you don't have all the information there, you first need all violations to e.g. group them. On the other hand violations have a very generic form at the moment, since ArchUnit is so powerful that it is hard to force a common message format on all the use cases. E.g. at the moment I can add my own condition to do

events.add(SimpleConditionEvent.violated(myCustomDomainObject, "Bummer"))`

And there is really not much structure within that message 😉
What I could imagine would be to enable a configurable violation format, i.e. a hook where you could add your own formatter

class MyViolationFormatter implements ViolationFormatter {
    @Override
    public List<String> formatViolationDetails(List<String> lines) {
        // do some string processing and regex magic
        return processedLines;
    }
}

Then maybe configure in archunit.properties

violationFormatter=com.myapp.architecture.MyViolationFormatter

The only bad thing is, that there is nothing available as input except a rough distinction between rule text and failure detail lines.
Something like your first request, to strip packages, should be okay to implement this way. For grouping dependencies a structure where originClass and targetClass would be present, would be way better of course. But then at that point you only have generic violation details that could come from any sort of violation and maybe not have a origin and target. If you know the message format, it's still possible to group by some parsing of course. But output text parsing is a pretty brittle API (even though ArchUnit's own violation detail format should be pretty stable meanwhile)...

[codecholeric]

The original problem regarding the heap space should be solved now by #138 and #326. I'll thus close this issue, because the part about violation formats has actually popped up here -> #301