[Bug] Msal Uses Common Fork Join Thread Pool as Default Thread Pool to execute Auth requests.

[g2vinay]

Library version used

1.15.0

Java version

Java 11

Scenario

PublicClient (AcquireTokenInteractive, AcquireTokenByUsernamePassword)

Is this a new or an existing app?

None

Issue description and reproduction steps

The Issue is that Msal Uses Default Common Fork Join Pool Thread Pool to execute auth requests, which results in deadlocks when high parallelism is used in customer apps.

This issue can be resolved by below approach:

1. Instantiate a static THREAD_POOL in the appropriate class where it can be accessed as below:

    // Use this THREAD_POOL in CompletableFuture to execute auth requests.
    public static final ExecutorService THREAD_POOL = getThreadPoolWithShutdownHook();
    private static final long THREADPOOL_SHUTDOWN_HOOK_TIMEOUT_SECONDS = 30;

2. Define the logic for getThreadPoolWithShutdownHook method:

    public static ExecutorService getThreadPoolWithShutdownHook() {
        return addShutdownHookSafely(Executors.newCachedThreadPool(new CustomThreadFactory("msal4j-client")),
            Duration.ofSeconds(THREADPOOL_SHUTDOWN_HOOK_TIMEOUT_SECONDS));
    }



    /**
     * Helper method that safely adds a {@link Runtime#addShutdownHook(Thread)} to the JVM that will close the
     * {@code executorService} when the JVM is shutting down.
     * <p>
     * {@link Runtime#addShutdownHook(Thread)} checks for security privileges and will throw an exception if the proper
     * security isn't available. So, if running with a security manager, setting
     * {@code AZURE_ENABLE_SHUTDOWN_HOOK_WITH_PRIVILEGE} to true will have this method use access controller to add
     * the shutdown hook with privileged permissions.
     * <p>
     * If {@code executorService} is null, no shutdown hook will be added and this method will return null.
     * <p>
     * The {@code shutdownTimeout} is the amount of time to wait for the {@code executorService} to shutdown. If the
     * {@code executorService} doesn't shutdown within half the timeout, it will be forcefully shutdown.
     *
     * @param executorService The {@link ExecutorService} to shutdown when the JVM is shutting down.
     * @param shutdownTimeout The amount of time to wait for the {@code executorService} to shutdown.
     * @return The {@code executorService} that was passed in.
     * @throws NullPointerException If {@code shutdownTimeout} is null.
     * @throws IllegalArgumentException If {@code shutdownTimeout} is zero or negative.
     */
    public static ExecutorService addShutdownHookSafely(ExecutorService executorService, Duration shutdownTimeout) {
        if (executorService == null) {
            return null;
        }
        Objects.requireNonNull(shutdownTimeout, "'shutdownTimeout' cannot be null.");
        if (shutdownTimeout.isZero() || shutdownTimeout.isNegative()) {
            throw new IllegalArgumentException("'shutdownTimeout' must be a non-zero positive duration.");
        }

        long timeoutNanos = shutdownTimeout.toNanos();
        Thread shutdownThread = new Thread(() -> {
            try {
                executorService.shutdown();
                if (!executorService.awaitTermination(timeoutNanos / 2, TimeUnit.NANOSECONDS)) {
                    executorService.shutdownNow();
                    executorService.awaitTermination(timeoutNanos / 2, TimeUnit.NANOSECONDS);
                }
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
                executorService.shutdown();
            }
        });

        CoreUtils.addShutdownHookSafely(shutdownThread);

        return executorService;
    }


   /**
     * Helper method that safely adds a {@link Runtime#addShutdownHook(Thread)} to the JVM that will run when the JVM is
     * shutting down.
     * <p>
     * {@link Runtime#addShutdownHook(Thread)} checks for security privileges and will throw an exception if the proper
     * security isn't available. So, if running with a security manager, setting
     * {@code AZURE_ENABLE_SHUTDOWN_HOOK_WITH_PRIVILEGE} to true will have this method use access controller to add
     * the shutdown hook with privileged permissions.
     * <p>
     * If {@code shutdownThread} is null, no shutdown hook will be added and this method will return null.
     *
     * @param shutdownThread The {@link Thread} that will be added as a
     * {@link Runtime#addShutdownHook(Thread) shutdown hook}.
     * @return The {@link Thread} that was passed in.
     */
    @SuppressWarnings({ "deprecation", "removal" })
    public static Thread addShutdownHookSafely(Thread shutdownThread) {
        if (shutdownThread == null) {
            return null;
        }

        if (ShutdownHookAccessHelperHolder.shutdownHookAccessHelper) {
            java.security.AccessController.doPrivileged((java.security.PrivilegedAction<Void>) () -> {
                Runtime.getRuntime().addShutdownHook(shutdownThread);
                return null;
            });
        } else {
            Runtime.getRuntime().addShutdownHook(shutdownThread);
        }

        return shutdownThread;
    }
    
    
    // Custom Thread Factory logic to give custom thread names.
    public class CustomThreadFactory implements ThreadFactory {
          private static final AtomicInteger poolNumber = new AtomicInteger(1);
          private final AtomicInteger threadNumber = new AtomicInteger(1);
          private final String namePrefix;
          public CustomThreadFactory(String namePrefix) {
              this.namePrefix = namePrefix + "-" +
                     poolNumber.getAndIncrement() +
                     "-thread-";
          }

         @Override
         public Thread newThread(Runnable r) {
              Thread thread = new Thread(r, namePrefix + threadNumber.getAndIncrement());
              if (thread.isDaemon())
                   thread.setDaemon(false);
              if (thread.getPriority() != Thread.NORM_PRIORITY)
                  thread.setPriority(Thread.NORM_PRIORITY);
              return thread;
         }
    }

Reference Issue by Az Identity customer: Azure/azure-sdk-for-java#39676

Relevant code snippets

No response

Expected behavior

No response

Identity provider

Microsoft Entra ID (Work and School accounts and Personal Microsoft accounts)

Regression

No response

Solution and workarounds

No response

[bgavrilMS]

Pls close the ICM: https://portal.microsofticm.com/imp/v3/incidents/incident/492551519/summary

[sonnyhcl]

which results in deadlocks when high parallelism is used in customer apps.

We've run into similar issue when using TokenCredential.getToken(). No obvious deadlock from thread dump, but the server is indeed stuck. Did a lot of investigation and repro, then find this thread.

Several questions to ask:

• Is there any clear explaination for the "deadlock" reason when using "ForkJoinPool.CommonPool" to execute auth requests?
• Is there any plan from MSAL side to avoid use ForkJoinPool.CommonPool?