Is it necessary to use AtomicInteger in a ThreadFactory?

Question

I think it's necessary to use AtomicInteger in the ThreadFactory but when I am trying to prove it to myself, I failed hard.

    new ThreadFactory() {

        private int threadId = 0;   <---- AtomicInteger preferred

        @Override
        public Thread newThread(Runnable r) {
            Thread t = new Thread(r);
            t.setDaemon(true);
            t.setName("my-thread-" + (threadId++));   <--- dangerous code
            return t;
        }
    }

If several requests come, the thread factory will generate threads to handle them and during the generation, there could be a gap during which a race condition might sneak in.

I tried with the following code to demonstrate my theory, but it's not happening at all with 2_000 core threads.

@Slf4j
public class ThreadFactoryTest {

    private ConcurrentHashMap<String, Thread> threadIdThreadMap = new ConcurrentHashMap<>();
    private ThreadPoolExecutor myExecutor = new ThreadPoolExecutor(2000, 2000, 30, TimeUnit.SECONDS,
            new ArrayBlockingQueue<>(100000), new ThreadFactory() {

        private int threadId = 0;

        @Override
        public Thread newThread(Runnable r) {
            Thread t = new Thread(r);
            t.setDaemon(true);
            t.setName("my-thread-" + (threadId++));
            if (threadIdThreadMap.contains(t.getName())) {
                log.error("already existed");
                System.err.println(myExecutor);
                myExecutor.shutdownNow();
            } else threadIdThreadMap.put(t.getName(), t);
            return t;
        }
    }, new ThreadPoolExecutor.AbortPolicy());

    @Test
    public void testThreadFactory() throws Exception {
        for (int i = 0; i < 100; ++i) {
            new Thread(() -> runOneHundredJobs()).start();
        }
        Thread.sleep(1000000);
        myExecutor.shutdown();
        myExecutor.awaitTermination(100, TimeUnit.MINUTES);
    }

    private void runOneHundredJobs() {
        log.info("{} starting to submit tasks", Thread.currentThread().getName());
        for (int i = 0; i < 100; ++i) {
            myExecutor.execute(() -> {
                while (100 < System.currentTimeMillis()) {
                    try {
                        Thread.sleep(1000);
                        if (Math.random() > 0.99) break;
                        System.out.println(Thread.currentThread().getName());
                        System.out.println(myExecutor);
                    } catch (Exception e) {

                    }
                }
            } );
        }
    }
}

Looks like a stupid question since I always know that "it's hard to create the gap for a multi-thread race condition".

Any help/clue will be appreciated ;)

UPDATE

Really thanks for the help along the way, @StephenC and @Slaw. I am sorry that I misunderstood some points there ;(

So newThread should be implemented in a thread-safe way and then in my case, the AtomicInteger is required. And I wanna have a quote from StephenC:

Failure to demonstrate a race condition doesn't mean it doesn't exist.

Answer 1

Is it necessary to use AtomicInteger in a ThreadFactory?

It will depend on how the factory object is used.

• If you supply a different factory object to each instance of ThreadPoolExecutor then the (actual) concurrency requirements for the factory will depend on how the executor uses it. In the absence of statements in the javadocs, you would need to examine the source code. I haven't checked, but I suspect that expansion of the thread pool (including the call to newThread) happen inside a mutex. If my suspicion is correct, then this use-case doesn't require the factory object to be thread-safe.

  UPDATE - I have now checked, and my suspicion was incorrect (for Java 8 & 12). The newThread call is made when creating a new Worker object, and that is not done while holding a mutex. Therefore, your newThread method needs to be thread-safe in this context too.

• If a factory object is shared with other things (e.g. another executor) then you are correct: your newThread method needs to be thread-safe.

---

I haven't looked at your code to try to show race conditions, but to my mind, that's not the best way to go about this. Code inspection and reasoning is a better way. Failure to demonstrate a race condition doesn't mean it doesn't exist.

Answer 2

I am making the test simpler to make the expected result emerge from under the water.

With the test below, the expected thread size is 1000 sharp while using int will give a less size oftentimes (994, 996, 999 in my macOS), not all the times.

public class ThreadFactoryTest {
    private ConcurrentHashMap<String, Thread> threadIdThreadMap = new ConcurrentHashMap<>();
    private ThreadPoolExecutor myExecutor = new ThreadPoolExecutor(2000, 2000, 30, TimeUnit.SECONDS,
            new ArrayBlockingQueue<>(100000), new ThreadFactory() {

        private int threadId = 0;
        private AtomicInteger atomicThreadId = new AtomicInteger(0);

        @Override
        public Thread newThread(Runnable r) {
            Thread t = new Thread(r);
            t.setDaemon(true);
            t.setName("my-thread-" + (threadId++));
            // uncomment this line, the thread size will be less than 1000
            t.setName("my-thread-" + (atomicThreadId.getAndIncrement()));
            threadIdThreadMap.put(t.getName(), t);
            return t;
        }
    }, new ThreadPoolExecutor.AbortPolicy());



    @Test
    public void testThreadFactory() throws Exception {
        for (int i = 0; i < 50; ++i) {
            new Thread(() -> runOneHundredJobs()).start();
        }
        Thread.sleep(1000000);
        myExecutor.shutdown();
        myExecutor.awaitTermination(100, TimeUnit.MINUTES);
    }

    private void runOneHundredJobs() {
        for (int i = 0; i < 20; ++i) {
            myExecutor.execute(() -> {
                while (100 < System.currentTimeMillis()) {
                    try {
                        Thread.sleep(1000);
                        log.warn("count: {}", threadIdThreadMap.size());
                    } catch (Exception e) {

                    }
                }
            });
        }
    }
}