Skip to content

Service Providers are not properly propagated to children #100

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

Closed
LexManos opened this issue Mar 1, 2022 · 3 comments · Fixed by McModLauncher/securejarhandler#52
Closed
Assignees
Milestone

Comments

@LexManos
Copy link
Contributor

LexManos commented Mar 1, 2022

The last argument to defineModules is a function that looks up a module, and it's associated class loader.
This should return the correct classloader for that specific module which is required to trigger bindToLayer, which is required for the service catalog.

Psudo of what the function should be:
f -> newConf.modules.contains(d) ? classLoader : parent.stream().findFirst(l -> l.contains(f)).map(Layer::getClassLoader).orElse(classLoader)
This causes service loaders for things like SLF4J to fail if they are first loaded from the MC context, which, in 1.18.2 they are.

@sciwhiz12 sciwhiz12 added the bug label Mar 1, 2022
@ichttt
Copy link
Member

ichttt commented Mar 3, 2022

I tried to implement it in the way you described. Unfortunally, this did not work. This is because Module.java only calls the function for modules of that classloader, and not it's parents.
I currently also don't see any other way to get the log4j2 service info into the transforming class loader.

@cpw
Copy link
Member

cpw commented Mar 6, 2022

Hmmm. We probably need to assume control of log4j loading anyway.

@marchermans
Copy link
Member

From testing myself, it looks like this is not directly possible with the current setup. As @ichttt already stated Module.java has a weird corner case here and I doubt we could modify its behavior enough to make it work. Optionally we could make the ServiceLoader use an adapted route through the classloader and combine all service specifications from the parent layers into one?

@marchermans marchermans self-assigned this Mar 21, 2022
@marchermans marchermans added this to the MC 1.19 milestone Mar 21, 2022
marchermans pushed a commit to McModLauncher/securejarhandler that referenced this issue Nov 21, 2023
…#52)

The fix is just to call the package-private
`ModuleLayer.bindToLoader(ClassLoader)` method on the parent layers when
creating the `ModuleClassLoader`.

The rest of the PR is a complicated testing setup, to make sure that we
can test with both CP-loaded and SJH-loaded source sets.

Closes McModLauncher/modlauncher#100.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants