[Fix #1611] Improving service loaders performance

[fjtirado]

Fix #1611

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Improves executor “service loader” performance by caching ServiceLoader results instead of repeatedly scanning the classpath on each builder/executor creation.

Changes:

• Cache HttpRequestDecorator lists per application id in HttpExecutorBuilder.
• Eagerly load/sort RunnableTaskBuilder implementations once in RunTaskExecutor.
• Eagerly load/sort ScriptRunner implementations once in RunScriptExecutorBuilder.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/HttpExecutorBuilder.java	Adds per-application caching of request decorators to avoid repeated ServiceLoader scans.
impl/core/src/main/java/io/serverlessworkflow/impl/executors/RunTaskExecutor.java	Preloads/sorts runnable task builders once to reduce repeated provider instantiation/sorting.
impl/core/src/main/java/io/serverlessworkflow/impl/executors/RunScriptExecutorBuilder.java	Preloads/sorts script runners once to avoid repeated ServiceLoader scans in build().

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Suppressed comments (3)

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowApplication.java:723

• ServiceLoader.load(clazz) uses the thread context classloader (TCCL). With the new caching, whichever TCCL is present on the first call effectively becomes 'sticky' for the lifetime of the application cache, which can lead to missing/incorrect service discovery in container/plugin environments. Consider using a stable classloader (e.g., ServiceLoader.load(clazz, WorkflowApplication.class.getClassLoader()) or a classloader stored on WorkflowApplication) so discovery is deterministic across threads.

  @SuppressWarnings("unchecked")
  public <T> List<T> serviceLoadedClasses(Class<T> clazz) {
    return (List<T>)
        serviceLoadedClasses.computeIfAbsent(
            clazz,
            c ->
                ServiceLoader.load(clazz).stream()
                    .map(ServiceLoader.Provider::get)
                    .sorted()
                    .toList());
  }

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/HttpExecutorBuilder.java:38

• requestDecorators was changed from final to non-final, but in this diff it’s still assigned only in the constructor. If the builder doesn’t reassign the list elsewhere, keeping it final (build the list in a local variable, then assign once) helps preserve immutability guarantees and reduces accidental reassignment risk.

  private List<HttpRequestDecorator> requestDecorators;

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/HttpExecutorBuilder.java:50

• requestDecorators was changed from final to non-final, but in this diff it’s still assigned only in the constructor. If the builder doesn’t reassign the list elsewhere, keeping it final (build the list in a local variable, then assign once) helps preserve immutability guarantees and reduces accidental reassignment risk.

    this.requestDecorators =
        new ArrayList<>(definition.application().serviceLoadedClasses(HttpRequestDecorator.class));

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/HttpExecutorBuilder.java:38

• requestDecorators no longer appears to be reassigned after construction (it’s still set once in the constructor). Keeping it final would better communicate immutability/intent and prevent accidental reassignment later.

  private List<HttpRequestDecorator> requestDecorators;

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

impl/core/src/main/java/io/serverlessworkflow/impl/auth/CommonOAuthProvider.java:99

• In the secret-based resolver, app.serviceLoadedClass(...) is executed every time the WorkflowValueResolver is evaluated. Even with caching, this adds repeated lookups and repeats the empty-list check + exception path setup on every call. Prefer resolving AccessTokenProviderFactory and JWTConverter once (outside the returned lambda) and capturing them in the closure so the resolver only performs the secret parsing and token-provider build.

      String secretName, AuthRequestBuilder authBuilder, WorkflowApplication app) {
    return (w, t, m) -> {
      Map<String, Object> secret = secret(w, secretName);
      String issuers = (String) secret.get("issuers");
      return app.serviceLoadedClass(AccessTokenProviderFactory.class)
          .build(
              authBuilder.apply(secret),
              issuers != null ? Arrays.asList(issuers.split(",")) : null,
              app.serviceLoadedClass(JWTConverter.class));
    };
  }

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/HttpExecutorBuilder.java:38

• If requestDecorators is only assigned in the constructor (as shown here), it should remain final to preserve immutability and make the builder state easier to reason about. Suggested fix: restore private final List<HttpRequestDecorator> requestDecorators; and keep the current initialization.

  private List<HttpRequestDecorator> requestDecorators;

impl/http/src/main/java/io/serverlessworkflow/impl/executors/http/HttpExecutorBuilder.java:50

• If requestDecorators is only assigned in the constructor (as shown here), it should remain final to preserve immutability and make the builder state easier to reason about. Suggested fix: restore private final List<HttpRequestDecorator> requestDecorators; and keep the current initialization.

    this.requestDecorators =
        new ArrayList<>(definition.application().serviceLoadedClasses(HttpRequestDecorator.class));

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowApplication.java:714

• The T extends Comparable<?> bound forces every SPI type loaded through these helpers to implement Comparable, even when ordering is not semantically required (e.g., singleton helpers where serviceLoadedClass(...) just returns the first). This makes the API more restrictive than necessary and can block future SPI extensions. Consider removing the bound and either (a) not sorting by default, (b) sorting only when T instanceof Comparable, or (c) providing an overload that accepts a Comparator<T> for the cases that require deterministic ordering.

  public <T extends Comparable<?>> List<T> serviceLoadedClasses(Class<T> clazz) {

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowApplication.java:725

• The T extends Comparable<?> bound forces every SPI type loaded through these helpers to implement Comparable, even when ordering is not semantically required (e.g., singleton helpers where serviceLoadedClass(...) just returns the first). This makes the API more restrictive than necessary and can block future SPI extensions. Consider removing the bound and either (a) not sorting by default, (b) sorting only when T instanceof Comparable, or (c) providing an overload that accepts a Comparator<T> for the cases that require deterministic ordering.

  public <T extends Comparable<?>> T serviceLoadedClass(Class<T> serviceClass) {

impl/core/src/main/java/io/serverlessworkflow/impl/WorkflowApplication.java:719

• Inside computeIfAbsent, the lambda parameter is c but the code uses the outer variable clazz. Using c would make it clearer that the load is based on the computed key and avoids accidental capture if this code is refactored. Suggested fix: replace ServiceLoader.load(clazz) with ServiceLoader.load(c).

        serviceLoadedClasses.computeIfAbsent(
            clazz,
            c ->
                ServiceLoader.load(clazz).stream()