ktlint hangs under certain conditions

[blindpirate]

Context

Recently, we (Gradle core team) observed some of our builds hang forever at this position:

"main" #1 prio=5 os_prio=31 tid=0x00007fa2d4002000 nid=0x1b03 waiting on condition [0x00007000010a3000]
   java.lang.Thread.State: WAITING (parking)
	at sun.misc.Unsafe.park(Native Method)
	- parking to wait for  <0x000000076b2c8450> (a java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
	at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
	at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(AbstractQueuedSynchronizer.java:2039)
	at java.util.concurrent.ArrayBlockingQueue.put(ArrayBlockingQueue.java:353)
	at com.github.shyiko.ktlint.Main.parallel(Main.kt:771)
	at com.github.shyiko.ktlint.Main.main(Main.kt:242)

I did some investigation and it seems like the current Main.parallel() implementation is not robust enough. Under certain conditions, it might hang forever.

Step to reproduce

I created a simple sample to reproduce it: blindpirate@cb161d0

Run the main method you can see the process hangs. Run jstack on the process you can see the process hangs at the exactly same position as the stacktrace above.

Analysis

When there're much more tasks than numberOfThreads and this line throws an exception, ArrayBlockingQueue.put() method may block forever because the consumer thread has already finished. Since the exception gets no chance to be rethrown, users can't see it at stderr and don't know what's happening there.

Possible solution

I don't know why ktlint uses such a complicated parallel implementation, can't we make it simpler, like blindpirate@14f37fd ?

[shyiko]

@blindpirate thank you for such a detailed issue!

I added a few comments (in addition to fixing the root cause) that should (hopefully) clarify the intent -https://github.com/shyiko/ktlint/blob/44fd18ccdd29b80a8aaa5301455c662c0d702532/ktlint/src/main/kotlin/com/github/shyiko/ktlint/Main.kt#L736-L794.
In short, cb(<task result>) needs to be executed in order in which tasks appear in the stream (i.e. tasks can run in parallel but results must be gathered in sequence). This is required so that ktlint would produce the same output no matter home many times it's re-run (otherwise you could get A.kt:1:1: error\nB.kt:1:1: error the first time and B.kt:1:1: error\nA.kt:1:1: error the next).

Regarding blindpirate@14f37fd:
seq.map { executorService.submit(it) }.forEach { cb(it.get()) } won't execute anything in parallel (Sequence.map is not terminal and so if you have n tasks execution sequence will be submit(task1) -> cb(task1 result) -> .. -> submit(taskN) -> cb(taskN result), not submit -> submit -> .. -> cb -> cb).
If you meant seq.toList().map(..).forEach()/seq.map(..).toList().forEach() then we'd end up buffering the whole sequence resulting in a lot of wasted time when doing something like ktlint --limit=10 (there is no need to traverse the fs once we've reached the error limit).

Fixed in 0.28.0.

[blindpirate]

Thank you so much for such a quick response!