Undertow - Only load resources that are known #43694

gsmet · 2024-10-04T11:08:17Z

If we try to load every resource, we end up creating cache entries in the resourceCache CHM of the AppClassLoader, which is definitely not what we want in this case, given we are supposed to serve only known resources.

Now I must admit the fix looks a bit too simple to be true :).

Fixes #43676

gsmet · 2024-10-04T11:09:02Z

@stuartwdouglas in case you have one minute to check if what I did is right or completely wrong, that would be appreciated. It looks like an oversight but you might have had very good reasons to not do that.

Interesting context is here: #43676 (comment) .

gsmet · 2024-10-04T12:36:17Z

OK, well, it doesn't look very good:

2024-10-04 11:25:20,035 ERROR [io.und.req.io] (executor-thread-1) Exception handling request b5951dd0-02d8-495a-8b76-5fcadb031235-1 to /foo/bar: java.lang.NullPointerException: Cannot invoke "java.util.Set.stream()" because "paths" is null
	at io.quarkus.undertow.test.ResourceManagerTestCase$ContextPathServlet.doGet(ResourceManagerTestCase.java:55)
	at jakarta.servlet.http.HttpServlet.service(HttpServlet.java:527)
	at jakarta.servlet.http.HttpServlet.service(HttpServlet.java:614)
	at io.undertow.servlet.handlers.ServletHandler.handleRequest(ServletHandler.java:74)
	at io.undertow.servlet.handlers.security.ServletSecurityRoleHandler.handleRequest(ServletSecurityRoleHandler.java:63)
	at io.undertow.servlet.handlers.ServletChain$1.handleRequest(ServletChain.java:68)
	at io.undertow.servlet.handlers.ServletDispatchingHandler.handleRequest(ServletDispatchingHandler.java:36)
	at io.undertow.servlet.handlers.RedirectDirHandler.handleRequest(RedirectDirHandler.java:67)
	at io.undertow.servlet.handlers.security.SSLInformationAssociationHandler.handleRequest(SSLInformationAssociationHandler.java:133)

I will have a second look after Devoxx.

gsmet · 2024-10-10T17:30:05Z

I pushed an additional commit (to be dropped) to get more information about the Windows CI failure.

gsmet · 2024-10-10T21:37:40Z

I had a closer look and we were actually collecting incorrect paths on Windows since forever but the previous code was actually fine with it. Now that we are stricter, we need to make sure the paths are correct.

gsmet · 2024-10-11T07:14:23Z

This should be ready now. We were doing shady things on Windows. I cleaned up things by making sure the paths are normalized from the start, instead of doing it on an ad hoc basis.

If we try to load every resource, we end up creating cache entries in the resourceCache CHM of the AppClassLoader, which is definitely not what we want in this case, given we are supposed to serve only known resources. Fixes quarkusio#43676

quarkus-bot · 2024-10-11T10:08:59Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 8d5680a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

melloware · 2024-10-13T23:52:53Z

@gsmet i think I helped cause some of this mess while trying to fix #28028 and created a test case with #33018

prateeksharma2988 · 2024-10-14T05:18:32Z

@gsmet Just for our reference, in which release version of quarkus, we should expect this fix would be available.
will it be available in 3.15 and 3.8 as LTS version update ?

gsmet · 2024-10-14T14:21:09Z

@prateeksharma2988 TBH the patch is a lot more involved than I thought it would be so I'm still unsure if we should backport it. It might get backported to 3.15 but I don't think it will hit 3.8.

Also we will need some bake time once 3.16 is released before we even think about backporting it.

gsmet · 2024-10-14T14:22:43Z

...ons/undertow/runtime/src/main/java/io/quarkus/undertow/runtime/KnownPathResourceManager.java

+        if (!files.contains(path)) {
+            return null;
+        }


The fix is actually here. The rest is to make paths consistent so that we don't end up comparing Unix-styled paths and Windows-style paths.

gsmet · 2024-10-14T14:24:54Z

@geoand @gastaldi I would appreciate another look at this one to make sure it makes sense.

The rationale of the patch is that in prod mode, we know which files are around and we have a fixed list of them and so we shouldn't try to resolve them from the class loader as we will end up caching all the negative hits, which might cause the CL to become huge if you have a lot of invalid requests.

I think that it was the original intent and that it somehow got lost.

geoand · 2024-10-14T14:27:30Z

I will take a look tomorrow

gastaldi

LGTM

geoand

Indeed looks like a legit omission :)

melloware · 2024-10-14T17:50:00Z

Can one of you kick off this build: #33018 I just rebased and want to see if it fixes that test case.

quarkus-bot bot added the area/undertow label Oct 4, 2024

gsmet added the triage/backport-3.15 label Oct 4, 2024

gsmet mentioned this pull request Oct 4, 2024

Undertow CacheResourceManager can cause frequent GC #43676

Closed

This comment has been minimized.

Sign in to view

gsmet marked this pull request as draft October 10, 2024 17:29

gsmet force-pushed the undertow-load-known branch 2 times, most recently from 79c7818 to ebb961b Compare October 11, 2024 07:08

gsmet marked this pull request as ready for review October 11, 2024 07:13

This comment has been minimized.

Sign in to view

gsmet added 3 commits October 11, 2024 09:27

Undertow - Normalize paths when collecting known static files

168aa0b

Undertow - Fix a misleading variable name

afbab96

Undertow - Only load resources that are known

8d5680a

If we try to load every resource, we end up creating cache entries in the resourceCache CHM of the AppClassLoader, which is definitely not what we want in this case, given we are supposed to serve only known resources. Fixes quarkusio#43676

gsmet force-pushed the undertow-load-known branch from ebb961b to 8d5680a Compare October 11, 2024 07:28

gsmet removed the triage/backport-3.15 label Oct 14, 2024

gsmet added the triage/backport-3.15 label Oct 14, 2024

gsmet commented Oct 14, 2024

View reviewed changes

gastaldi approved these changes Oct 14, 2024

View reviewed changes

melloware approved these changes Oct 14, 2024

View reviewed changes

geoand approved these changes Oct 14, 2024

View reviewed changes

gsmet merged commit 46f6063 into quarkusio:main Oct 14, 2024
48 checks passed

quarkus-bot bot added this to the 3.16 - main milestone Oct 14, 2024

melloware mentioned this pull request Oct 14, 2024

Undertow: servletContext.getResourcePaths("/") not listed in PROD mode #28028

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undertow - Only load resources that are known #43694

Undertow - Only load resources that are known #43694

gsmet commented Oct 4, 2024

gsmet commented Oct 4, 2024 •

edited

Loading

gsmet commented Oct 4, 2024

This comment has been minimized.

gsmet commented Oct 10, 2024

gsmet commented Oct 10, 2024

This comment has been minimized.

gsmet commented Oct 11, 2024 •

edited

Loading

quarkus-bot bot commented Oct 11, 2024 •

edited by github-actions bot

Loading

melloware commented Oct 13, 2024

prateeksharma2988 commented Oct 14, 2024 •

edited

Loading

gsmet commented Oct 14, 2024

gsmet Oct 14, 2024

gsmet commented Oct 14, 2024

geoand commented Oct 14, 2024

gastaldi left a comment

geoand left a comment

melloware commented Oct 14, 2024

Undertow - Only load resources that are known #43694

Undertow - Only load resources that are known #43694

Conversation

gsmet commented Oct 4, 2024

gsmet commented Oct 4, 2024 • edited Loading

gsmet commented Oct 4, 2024

This comment has been minimized.

gsmet commented Oct 10, 2024

gsmet commented Oct 10, 2024

This comment has been minimized.

gsmet commented Oct 11, 2024 • edited Loading

quarkus-bot bot commented Oct 11, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

melloware commented Oct 13, 2024

prateeksharma2988 commented Oct 14, 2024 • edited Loading

gsmet commented Oct 14, 2024

gsmet Oct 14, 2024

Choose a reason for hiding this comment

gsmet commented Oct 14, 2024

geoand commented Oct 14, 2024

gastaldi left a comment

Choose a reason for hiding this comment

geoand left a comment

Choose a reason for hiding this comment

melloware commented Oct 14, 2024

gsmet commented Oct 4, 2024 •

edited

Loading

gsmet commented Oct 11, 2024 •

edited

Loading

quarkus-bot bot commented Oct 11, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`

prateeksharma2988 commented Oct 14, 2024 •

edited

Loading