AzureAD / microsoft-authentication-library-for-java

Microsoft Authentication Library (MSAL) for Java http://aka.ms/aadv2
MIT License
280 stars 137 forks source link

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

Open g2vinay opened 2 months ago

g2vinay commented 2 months ago

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: https://github.com/Azure/azure-sdk-for-java/issues/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 commented 2 months ago

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