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

Improve class loader isolation #630

Merged
merged 8 commits into from
Apr 2, 2022
Merged

Improve class loader isolation #630

merged 8 commits into from
Apr 2, 2022

Conversation

sfPlayer1
Copy link
Contributor

With this PR Loader should achieve proper isolation between the system/app class loader and the transforming class loader, hopefully eliminating remaining shadowing issues outside rather intentional misbehavior. All game and mod content is supposed to be on the transforming class loader, only Loader and its core libs should be loaded from the app CL.

A side effect of this effort is that the entire game including libraries becomes exposed to Mixin and other bytecode editing. This allows more direct patching of something like the Log4J issue in the future.

LibClassifier has been refactored to become usable by 3rd party game providers.

Mod class path handling still needs to be finalized.

Copy link

@LoganDark LoganDark left a comment

Choose a reason for hiding this comment

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

Had to make a couple changes to build this

@LoganDark
Copy link

Fixes #190

@liach liach requested a review from modmuss50 March 29, 2022 05:11
@sfPlayer1 sfPlayer1 marked this pull request as ready for review March 29, 2022 06:06
@LoganDark
Copy link

LoganDark commented Mar 29, 2022

image

Is there a reason for this log message having a duplicate, and printing without color the first time?

The current latest release of Loader does not do that:

image

Maybe it has to initialize a little bit, then jump onto the transforming class loader to do the rest? Perhaps I have no idea what I'm talking about but that would be my guess.

P.S. While the colors do work now, there are still a ton of errors that probably need to be fixed before this can be merged:

image

@sfPlayer1
Copy link
Contributor Author

The first line is from before log4j is initialized, it replays any previous output to ensure everything went to the log file

@LoganDark
Copy link

LoganDark commented Mar 29, 2022

The first line is from before log4j is initialized, it replays any previous output to ensure everything went to the log file

Ah, so log4j isn't loaded until the transforming class loader is ready, right? Maybe the log level should be set to warn until log4j can replay the info messages

@sfPlayer1
Copy link
Contributor Author

Yes, there is nothing warning-like about this - it is expected behavior unless we decide to hide the early output, which has its own problems

@LoganDark
Copy link

Yes, there is nothing warning-like about this - it is expected behavior unless we decide to hide the early output, which has its own problems

As long as early failures are printed (like setting the log level to warn instead of completely silencing it), I would actually prefer if the early log messages waited to be printed for log4j, since log4j is prettier. (also duplicate messages)

@LoganDark
Copy link

LoganDark commented Apr 2, 2022

StatusLogger errors are finally gone. Reposting a message I posted to the Discord:

my proposal:

  • silence all non-ERROR messages before log4j is initialized
  • once log4j is initialized then let it use the normal level as usual and replay the logs that weren't printed before

this means errors before log4j is initialized will still be displayed if log4j has no opportunity to replay them, but other stuff can wait till the replay

AFAICT, that's one of the only things left to do before merging this PR. Totally optional, but it would look a lot cleaner, and bring the output logs back up to parity with the current version 0.13.3.

@liach
Copy link
Contributor

liach commented Apr 2, 2022

does slf4j replay logs?

@sfPlayer1
Copy link
Contributor Author

slf4j doesn't solve the issue where the backend may not be ready yet, either way the logging thing is old behavior and to be tweaked separately

@LoganDark
Copy link

the logging thing is old behavior

I specifically tested the previous version of Loader and it doesn't have that - it's definitely introduced by this PR.

@sfPlayer1
Copy link
Contributor Author

For some cases, yes, but I removed them as it'd involve using Log4J from the system class path, which is not what we want to keep doing since it prevents patching it and pulls all deps with it. It did however behave exactly like now in other circumstances. Don't worry though, I plan to land a PR to delay the log output before the next Loader release.

VelizarBG added a commit to VelizarBG/suggestion-tweaker that referenced this pull request Apr 10, 2023
VelizarBG added a commit to VelizarBG/suggestion-tweaker that referenced this pull request Apr 10, 2023
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.

None yet

4 participants