Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClassLoader and augmentation issues #770

Closed
FroMage opened this issue Feb 6, 2019 · 12 comments
Closed

ClassLoader and augmentation issues #770

FroMage opened this issue Feb 6, 2019 · 12 comments
Labels
kind/bug Something isn't working
Milestone

Comments

@FroMage
Copy link
Member

FroMage commented Feb 6, 2019

For example, Hibernate and Panache enhance entity Foo, and Panache then generates a Foo$generated class, which uses reflection on Foo to get some info that's hard to get using jandex. It doesn't care if it sees a non-enhanced version of the class, using its class' class loader. But if it uses theTCCL (RuntimeClassLoader) that will trigger a load of the entity class before the transformers have been set and all hell breaks loose later on with really weird failures.

And if I don't use the TCCL shamrock:dev does not work, for some other reason.

We're going ot have to clarify what reflection is allowed during bytecode generation (Hibernate uses it, ASM uses it), and how to avoid these issues of difference between shamrock:dev and the normal runner.

@FroMage FroMage added the kind/bug Something isn't working label Feb 6, 2019
@dmlloyd
Copy link
Member

dmlloyd commented Feb 6, 2019

We shouldn't rely on TCCL IMO.

Once the new class loading model is in place, we probably should use build items: one for the extension deployment CL, one for the extension runtime CL, one for the bootstrap CL, and one for the CL used for reflective instrumentation of the original user classes.

@FroMage
Copy link
Member Author

FroMage commented Feb 6, 2019

Are we really going towards a model with 4 CL?

@dmlloyd
Copy link
Member

dmlloyd commented Feb 6, 2019

Yes. To be fair though, one of them is the original one that we'd have anyway.

@dmlloyd
Copy link
Member

dmlloyd commented Feb 6, 2019

And one would only be created if an extension were present which needed to do reflection on the app classes.

@dmlloyd
Copy link
Member

dmlloyd commented Feb 6, 2019

Here's a helpful diagram:

  ┌───────────────────────────────┐
  │   Build system class loader   │         ┌───────────────────────────────┐
  │  ┌─────────────────────────┐  │         │     Runtime class loader      │
  │  │     Application JAR     │  │         │  ┌─────────────────────────┐  │
  │  │     (orig. version)     │  │         │  │      quarkus-core       │  │
  │  ├─────────────────────────┤  │         │  │        (curated)        │  │
  │  │ Quarkus Build Bootstrap │─ ┼ ─ ─ ┐   │  ├─────────────────────────┤  │
  │  │                         │◀─┼──┬──────│  │   quarkus-* (runtime)   │  │◀───────────────┐
  │  ├─────────────────────────┤  │  │  │   │  │        (curated)        │  │                │
  │  │      JBoss Logging      │◀─┼──┤      │  ├─────────────────────────┤  │ ┌─────────────────────────────┐
  │  ├─────────────────────────┤  │  │  │   │  │ Framework dependencies  │  │ │  Augmentation class loader  │
  │  │    JBoss LogManager     │◀─┼──┤      │  └─────────────────────────┘  │ │     (created on demand)     │
  │  ├─────────────────────────┤  │  │  │   └───────────────────────────────┘ │ ┌─────────────────────────┐ │
  │  │     WildFly Common      │◀─┼──┘                      ▲                 │ │     Application JAR     │ │
  │  ├─────────────────────────┤  │     │                   │                 │ │       (original)        │ │
  │  │      quarkus-core       │  │  ┌──────────────────────┴────────┐        │ ├─────────────────────────┤ │
  │  │     (orig. version)     │  │  │  │                            │        │ │Application dependencies │ │
  │  ├─────────────────────────┤  │  │                               │        │ └─────────────────────────┘ │
  │  │   quarkus-* (runtime)   │  │  │  │                            │        └─────────────────────────────┘
  │  │    (orig. versions)     │  │  │               ┌───────────────────────────────┐       ▲
  │  ├─────────────────────────┤  │  │  └ ─ ─ ─ ─ ─ ▶│    Deployment class loader    │
  │  │Application and framework│  │  │               │  ┌─────────────────────────┐  │       │
  │  │   dependencies (orig.   │  │  │               │  │ quarkus-core-deployment │  │
  │  │        versions)        │  │  │               │  │        (curated)        │─ ┼ ─ ─ ─ ┘
  │  └─────────────────────────┘  │  │               │  ├─────────────────────────┤  │
  └───────────────────────────────┘  │               │  │  quarkus-*-deployment   │  │
                                     │         ┌ ─ ─ ┼ ─│        (curated)        │  │
                    ┌────────────────┘               │  └─────────────────────────┘  │
                    │                          │     └───────────────────────────────┘
  ┌───────────────────────────────────┐
  │       Test/Dev class loader       │        │
  │  ┌─────────────────────────────┐  │
  │  │     Application Classes     │  │        │
  │  │      (augmented, RAM)       │  │
  │  ├─────────────────────────────┤  │◀ ─ ─ ─ ┘
  │  │  Application and framework  │  │
  │  │dependencies (augmented, RAM)│  │
  │  └─────────────────────────────┘  │
  └───────────────────────────────────┘

@FroMage
Copy link
Member Author

FroMage commented Feb 6, 2019

I love how the world has moved on to super cool looking diagrams with animations, 3D and gradients, and we've been left to play with ascii diagrams :)

@FroMage
Copy link
Member Author

FroMage commented Feb 6, 2019

Shit I'm old enough to have started working on Borland C under DOS which looked exactly like this diagram!

@FroMage
Copy link
Member Author

FroMage commented Feb 6, 2019

Note that I'm not criticising, I would have used Dia and it would have looked much worse.

@dmlloyd
Copy link
Member

dmlloyd commented Feb 6, 2019

Haha. I saw this program "Monodraw" and wanted to give it a try.

@dmlloyd
Copy link
Member

dmlloyd commented Feb 12, 2019

Depends on #425

@rsvoboda
Copy link
Member

@FroMage is this fixed by the big CL change commit ?

@FroMage
Copy link
Member Author

FroMage commented Jan 24, 2020

Yeah, at the very least it clarified how things work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants