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.

Sign up for GitHub

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

Aggressively clear the fields of the QuarkusClassLoader on close #41608

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Aggressively clear the fields of the QuarkusClassLoader on close #41608

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e66b77
Aggressively clear the fields of the QuarkusClassLoader on close
gsmet Jun 12, 2024
dc97790
Adjust how we close the CL and throw an exception when closed
gsmet Jul 6, 2024
c16e909
Call ensureOpen() a bit later in loadClass()
gsmet Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 37 additions & 18 deletions ...ts/bootstrap/core/src/main/java/io/quarkus/bootstrap/classloading/QuarkusClassLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,8 +511,6 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {

@Override
protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
ensureOpen();

for (ClassLoaderEventListener l : classLoaderEventListeners) {
l.loadClass(name, this.name);
}
Expand All @@ -529,6 +527,9 @@ protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundE
if (c != null) {
return c;
}

ensureOpen();

String resourceName = fromClassNameToResourceName(name);
if (state.bannedResources.contains(resourceName)) {
throw new ClassNotFoundException(name);
Expand Down Expand Up @@ -643,8 +644,6 @@ public List<String> getLocalClassNames() {
}

public Class<?> visibleDefineClass(String name, byte[] b, int off, int len) throws ClassFormatError {
ensureOpen();

return super.defineClass(name, b, off, len);
}

Expand Down Expand Up @@ -692,17 +691,28 @@ public void close() {
log.debug("Failed to clean up DB drivers");
}
}
for (ClassPathElement element : elements) {
//note that this is a 'soft' close
//all resources are closed, however the CL can still be used
//but after close no resources will be held past the scope of an operation
try (ClassPathElement ignored = element) {
//the close() operation is implied by the try-with syntax
} catch (Exception e) {
log.error("Failed to close " + element, e);
}

status = STATUS_CLOSED;

closeClassPathElements(elements);
closeClassPathElements(bannedElements);
closeClassPathElements(parentFirstElements);
closeClassPathElements(lesserPriorityElements);

definedPackages.clear();
resettableElement = null;
transformedClasses = null;
if (state != null) {
state.clear();
}
for (ClassPathElement element : bannedElements) {
closeTasks.clear();
classLoaderEventListeners.clear();

ResourceBundle.clearCache(this);
}

private static void closeClassPathElements(List<ClassPathElement> classPathElements) {
for (ClassPathElement element : classPathElements) {
//note that this is a 'soft' close
//all resources are closed, however the CL can still be used
//but after close no resources will be held past the scope of an operation
Expand All @@ -712,21 +722,25 @@ public void close() {
log.error("Failed to close " + element, e);
}
}
ResourceBundle.clearCache(this);

status = STATUS_CLOSED;
classPathElements.clear();
Copy link
Member

Choose a reason for hiding this comment

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

if we are doing that we could as well simply de-reference those instead of clearing

Copy link
Member Author

Choose a reason for hiding this comment

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

Setting them to null would probably be problematic when we access the CL after it has been closed.

I'm not exactly optimistic on our capability to fix all the problematic cases soonish (if ever).

Copy link
Member

Choose a reason for hiding this comment

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

you could use List.of() but i am not sure an empty list will actually work, we'll see what the CI says

}

public boolean isClosed() {
return status < STATUS_OPEN;
}

private void ensureOpen() {
if (LOG_ACCESS_TO_CLOSED_CLASS_LOADERS && status == STATUS_CLOSED) {
if (status != STATUS_CLOSED) {
return;
}

if (LOG_ACCESS_TO_CLOSED_CLASS_LOADERS) {
// we do not use a logger as it might require some class loading
System.out.println("Class loader " + this + " has been closed and may not be accessed anymore");
Thread.dumpStack();
}

throw new IllegalStateException("Class loader " + this + " has been closed and may not be accessed anymore");
}

@Override
Expand Down Expand Up @@ -900,6 +914,11 @@ static final class ClassLoaderState {
this.bannedResources = bannedResources;
this.parentFirstResources = parentFirstResources;
}

void clear() {
// when the CL is closed, we make sure the resources are not loadable anymore
loadableResources.clear();
}
}

@Override
Expand Down