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

ktlint hangs under certain conditions #277

Closed
blindpirate opened this issue Sep 5, 2018 · 2 comments
Closed

ktlint hangs under certain conditions #277

blindpirate opened this issue Sep 5, 2018 · 2 comments

Comments

@blindpirate
Copy link

blindpirate commented Sep 5, 2018

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 shyiko closed this as completed in 44fd18c Sep 5, 2018
@shyiko
Copy link
Collaborator

shyiko commented Sep 5, 2018

@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
Copy link
Author

Thank you so much for such a quick response!

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

No branches or pull requests

2 participants