Explicitly state that maxThreads does not control created thread count#481
Explicitly state that maxThreads does not control created thread count#481
Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!What happened?Your changelog entries have been stored in the database as part of our migration to ChangelogV3. Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. |
| * The maximum number of threads that were running at once. A thread is counted as running if it has a start | ||
| * instant but not yet an end instant. | ||
| */ | ||
| private static ThreadPeak peakThreads(List<Instant> starts, List<Instant> ends) { |
There was a problem hiding this comment.
Using wall-clock time here is brittle. A better implementation would be to use a counter that keeps track of the largest value it has seen.
| .succeedsWithin(Duration.ofSeconds(10)) | ||
| .asInstanceOf(InstanceOfAssertFactories.list(String.class)) | ||
| .hasSize(tasks) | ||
| .allSatisfy(value -> assertThat(value).startsWith("foo-")); |
There was a problem hiding this comment.
There's no need for a thread return value, this assertion isn't useful.
| List<ListenableFuture<String>> futures = new ArrayList<>(tasks); | ||
| List<Instant> threadStarts = Collections.synchronizedList(new ArrayList<>(maxThreads)); | ||
| List<Instant> threadEnds = Collections.synchronizedList(new ArrayList<>(maxThreads)); | ||
| ListeningExecutorService executor = MoreExecutors.listeningDecorator(NylonExecutor.builder() |
There was a problem hiding this comment.
There's no need for a listening decorator here
| assertThat(Futures.successfulAsList(futures)) | ||
| .succeedsWithin(Duration.ofSeconds(10)) | ||
| .asInstanceOf(InstanceOfAssertFactories.list(String.class)) | ||
| .hasSize(tasks) |
There was a problem hiding this comment.
Just assert each of these in a loop. 10 second is also far too long.
futures.forEach(future -> {
future.succeedsWithin(1, TimeUnit.SECOND);
});
| countDownLatch.countDown(); | ||
| countDownLatch.await(); |
There was a problem hiding this comment.
CyclicBarrier feels more appropriate here
| countDownLatch.countDown(); | |
| countDownLatch.await(); | |
| barrier.await(); |
| assertThat(delegate) | ||
| .asInstanceOf(InstanceOfAssertFactories.type(ThreadPoolExecutor.class)) | ||
| .satisfies(threadPoolExecutor -> { | ||
| assertThat(threadPoolExecutor.getCompletedTaskCount()).isEqualTo(tasks); | ||
| }); |
There was a problem hiding this comment.
This seems redundant - we've asserted that all the futures have succeeded.
2 participants
Before this PR
There was some confusion in thread about what the
maxThreadsparameter does.After this PR
==COMMIT_MSG==
Explicitly state that maxThreads does not control created thread count
==COMMIT_MSG==
This PR adds further Javadoc about the maxThread parameter, and adds a test demonstrating that behavior.
Possible downsides?