Skip to content

WIP fix: Isolate codegen classpath from ModelAssembler#1705

Closed
ghostbuster91 wants to merge 22 commits into
disneystreaming:series/0.18from
ghostbuster91:classpath-isolation
Closed

WIP fix: Isolate codegen classpath from ModelAssembler#1705
ghostbuster91 wants to merge 22 commits into
disneystreaming:series/0.18from
ghostbuster91:classpath-isolation

Conversation

@ghostbuster91

@ghostbuster91 ghostbuster91 commented Apr 23, 2025

Copy link
Copy Markdown
Contributor

What problem does this solve?

This solves two problems.
First, there is an inconsistency in the current code regarding how the shapes are loaded vs how classes are loaded.

We look for shapes manually via:

   val modelsInJars = deps.flatMap { file =>
      Using.resource(
        // Note: On JDK13+, the second parameter is redundant.
        FileSystems.newFileSystem(file.toPath(), null: ClassLoader)
      ) { jarFS =>
        val p = jarFS.getPath("META-INF", "smithy", "manifest")

        // model discovery would throw if we tried to pass a non-existent path
        if (!Files.exists(p)) Nil
        else {
          try ModelDiscovery.findModels(p.toUri().toURL()).asScala.toList
          catch {
            case e: ModelManifestException =>
              System.err.println(
                s"Unexpected exception while loading model from $file, skipping: $e"
              )
              Nil
          }

This scans only external jars. It will not discover shapes on the current classpath.

However, the validatorClassLoader that is passed to ModelAssembler is constructed as:

    val validatorClassLoader = locally {
      val jarUrls = deps.map(_.toURI().toURL()).toArray
      new URLClassLoader(jarUrls, currentClassLoader)
    }

Which means that SPI will load matching implementation also from the current classpath, because the currentClassLoader is the parent of the validatorClassLoader.

So it might be possible that a java part of the shape, such as TraitService$Provider, is found but its Smithy counterpart is not. This might be confusing.

Second, because currentClassLoader is the parent of validatorClassLoader any class that is on the currentClassLoader classpath will be loaded by that classloader. This can lead to a very surprising behaviors if a class is changed between the versions of a jar that belongs to both classloaders.

Example:

Smithy4sCodegen depends on protocol module, so it has it on its classpath.
protocol module defines AdtMemberTraitValidator.
Now, if you try to generate code for a smithy artifact that depends on another version of the protocol that has a different version of AdtMemberTraitValidator you won't see that version but instead you will see the one from the codegen classpath because of the parent-first delegation rule.


This PR currently fails to build. I suppose that it is due to some missing transformations from the current classpath that need to be somehow added into the isolated loader.

An alternative approach to this would be to create a proxy using reflection and have a full isolation but I believe that it is too cumbersome.

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@Baccata

Baccata commented Apr 24, 2025

Copy link
Copy Markdown
Contributor

I appreciate and support an effort to isolate the smithy pipeline away from the program's main classloader, so long as you manage to get it to build :s

@ghostbuster91

ghostbuster91 commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

Summary

It is possible to achieve better isolation between the codegen's classpath and model validator's classpath. In the current approach the common denominator is smithy-model api that is shared between model validator classloader and the codegen's classloader.

This has the consequence that 3rd party shapes that are present in both classloaders can be no longer queried by class, e.g. member.hasTrait(classOf[alloy.NullableTrait]) and have to be queried by smithy shape instead member.hasTrait(alloy.NullableTrait.ID)

A class in JVM is identified by its FQN and the classloader that loaded it.

Codegen protocol

Codegen module used to define some transformations. Unless they are explicitly added into the bridging classloader they can't be seen by the model validator classloader. Instead I decided to extract them into a separate module that is passed together with the protocol module into the model assembly as external jar.

Openapi

One thing that I was not aware of before starting this work is that the codegen renders also openapi files using alloy-openapi module.

This breaks once we put the stronger isolation in place because the smithy-openapi module compares traits by classes, not by id.

One way to fix it is to actually rely on alloy-open being provided by user. If there is no such module provided than the codegen could simply skip openapi generation step. In order for arlib to use the provided classloader a small change is required alloy.

I also explored the other approach where the codegen uses its own alloy-openapi to generate openapi files. This requires some changes in alloy and in smithy. Surely, the implementation might be more polished, I just wanted to get it working. The change itself might be desired but I am less in favor of this approach.

resolve: Boolean
): Class[?] = {
if (
name.startsWith("software.amazon.smithy") || name.startsWith("scala")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks fragile but maybe it won't have to be changed at all?

@ghostbuster91

Copy link
Copy Markdown
Contributor Author

just sbt scripted tests are failing due to the use of alloy snapshot

@ghostbuster91

Copy link
Copy Markdown
Contributor Author

closing in favor of #1707

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants