Simplify `ConcurrentRunnerScheduler` & cleanup.

Review Request #4091 — Created July 18, 2016 and submitted

jsirois
pants
jsirois/issues/3684
3684, 3686
10c3e95...
pants-reviews
zundel
The intermediary `CompletionService` and `concurrentTasks` queue were
un-needed.  This excises both in favor of leveraging the contracts of
`ExecutorService` and ensures cleanup of the `ExecutorService` as well,
which prevents hundreds of threads being spawned and not joined in unit
tests.

 src/java/org/pantsbuild/tools/junit/impl/ConcurrentRunnerScheduler.java | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)
Tested locally with a local `~/.m2/repository` publish of the junit
tool jar and repeated `jstack` invocations confirmed a steady state of
0 "concurrent-junit-runner-*" threads vs the 100s observed previously.

CI went green here:
  https://travis-ci.org/pantsbuild/pants/builds/145661315
  1. Hey John, are you relying on parallel execution at all? If so, I'd love it if you would try enabling --test-junit-use-experimental-runner.

    I'm asking @chiester to take a look at this too. He's also done a lot of work in here recently.

    1. I'm not - this is me trying to get jenkins CI green.
    2. We should definitely check if the experimental runner is leaking threads or not.

    3. By inspection it doesn't leak, but it probably creates way too many. Seems to me private final ExecutorService fService = Executors.newFixedThreadPool(numParallelThreads); should be moved out to the ConcurrentComputer constructor for sanity sake.

    4. ...which will have a good deal of internal fallout, but relying on their being exactly 1 parent runner, at least without comment, reads as perilous.
  2. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit 8e1907efcd4424db03518d59d2bb81bc7e62664d
Author: John Sirois <john.sirois@gmail.com>
Date:   Mon Jul 18 16:50:04 2016 -0600

    Simplify `ConcurrentRunnerScheduler` & cleanup.
    
    The intermediary `CompletionService` and `concurrentTasks` queue were
    un-needed.  This excises both in favor of leveraging the contracts of
    `ExecutorService` and ensures cleanup of the `ExecutorService` as well,
    which prevents hundreds of threads being spawned and not joined in unit
    tests.
    
    Testing Done:
    Tested locally with a local `~/.m2/repository` publish of the junit
    tool jar and repeated `jstack` invocations confirmed a steady state of
    0 "concurrent-junit-runner-*" threads vs the 100s observed previously.
    
    CI went green here:
      https://travis-ci.org/pantsbuild/pants/builds/145661315
    
    Bugs closed: 3684, 3686
    
    Reviewed at https://rbcommons.com/s/twitter/r/4091/
Loading...