[8.0.x] Another TCCL issue about servlet bundles

[qianwch]

I have tested version 8.0.9. Still has problem.

1. TCCL now includes the servlet bundle. But the TCCL is not the same as servletContext.getClassLoader()
2. Even worse, TCCL includes every servlet bundles, so the class space collides between the servlet bundles. easily get class cast exceptions.
   Here are the two bundles that will demo the class space collision problem:
   demo1.zip
   demo2.zip

And it is the TCCL:

Originally posted by @qianwch in #1759 (comment)

[qianwch]

I can not reopen the old issue, so create a new.
@grgrzybek

[grgrzybek]

Let me try to explain why I think it looks fine ;)

about TCCL containing demo-pax1 and demo-pax2 bundles:

You've installed two bundles into the same context so effectively the context runs servlets from different sources/bundles/origins that's why the only option is to see both bundles. Otherwise the Jetty (or Tomcat or Undertow) servlet handler which manages servlets for given context would have to set up a TCCL with a bundle for target servlet. But how about filters? They may come from different bundles as well - same for forward and include requests.

That's why I believe it's now the developer's responsibility to not include the same servlet class in two different bundles. It's like WAR with JARs in WEB-INF/lib - I'd rather not expect two jars to provide the same servlet for given WAR.

about TCCL being different than servletContext.getClassLoader():

While TCCL is set up according to the above, servlet context's classloader is set according to Whiteboard specification - https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.http.whiteboard.html#d0e119708:

getClassLoader() […] Returns the class loader of the bundle that registered the Whiteboard service. An implementation of this specification can achieve this by returning separate façades of the ServletContext to each Whiteboard service. Each façade accesses the Whiteboard service's Bundle Wiring to obtain its class loader.

Pax Web explicitly creates such ServletConfig and request wrappers, so their ServletContext overrides getClassLoader() method.

I found a bit more such vague places in Whiteboard/HttpService/WebApplications OSGi CMPN specifications and I had to make some compromises so Servlet API specification is still not (much) violated.

I hope you understand my design decisions. And thank you very much for detailed analysis ;)

[qianwch]

@grgrzybek Thank you for your explanation. Now I understand it.
So the Filters in whiteboard should NOT work at all without your new fix.
But I have tested in 8.0.6 which does not include servlet/filter bundles in TCCL at all, it worked.
I also tested forwarding from servlet to another servlet in another bundle of the same servlet context, if worked too.

So may be it is just fine to set the TCCL the same as servletContext.getClassLoader(). Could you please reconsider it?

[grgrzybek]

So the Filters in whiteboard should NOT work at all without your new fix.

It depends what you're trying to do there ;) All the filters I tested (including 3rd party ones from Jolokia, Hawtio, MyFaces, PrimeFaces, Spring, ...) worked fine. Simply doGet()/service() and doFilter() methods are usually not the best places to load classes :)
initialization of servlets/filters should be done with correct TCCL.
In OSGi you can register a servlet to already started context, so my fix ensures that the servlet context's class loader which is set up as TCCL contains all the bundles of all registered servlets/filters/listeners/websockets).
Before the fix, servletContext.getClassLoader() was set up according to Whiteboard specification. I simply didn't check TCCL at all.

But I have tested in 8.0.6 which does not include servlet/filter bundles in TCCL at all, it worked.

But you tested the example with:

	public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
		response.getWriter().println("<h1>it worked</h1>");
		try {
			Thread.currentThread().getContextClassLoader().loadClass(MyServlet.class.getName());
		} catch (ClassNotFoundException e) {
			throw new RuntimeException(e);
		}
	}

?
What was working in 8.0.6?

[qianwch]

What was working in 8.0.6?

No, it does not work. It is what your new patch fixes.
The Filter's doFilter and Servlet's request.getRequestDispatcher("/xxxx").forward/include from one bundle to another bundle are fine without your fix.
So I think TCCL should include the servlet bundle requested, but need not and should not include other servlet/filter bundles.

[grgrzybek]

So I think TCCL should include the servlet bundle requested, but need not and should not include other servlet/filter bundles.

As I said - it's not defined at all in Whiteboard (and entire OSGi CMPN) specification, so we have to make some sacrifices. I don't see an easy way to ensure that...

Even in WAB scenario, part of Pax Web processing is inclusion of all reachable bundles. The reachability covers:

• jars from Bundle-ClassPath manifest header
• bundles wired using Import-Package
• bundles wired using Require-Bundle
• fragments wired using Fragment-Host

Eventually, the reachable bundles/jars constitute a "class space", so potential conflicts should be resolved by the developer...

If you have time, please create a PR and add a sample (samples) to https://github.com/ops4j/org.ops4j.pax.web/tree/main/samples/samples-whiteboard showing what you would like to achieve and we'll think what can be done.

[qianwch]

As I said - it's not defined at all in Whiteboard (and entire OSGi CMPN) specification, so we have to make some sacrifices. I don't see an easy way to ensure that...

Do we both agree that TCCL should include the servlet bundle requested, but need not and should not include other servlet/filter bundles? If so, I would take some time to figure out how to accomplish that.

Even in WAB scenario, part of Pax Web processing is inclusion of all reachable bundles. The reachability covers:

I don't think it is the same scenario as whiteboard. The reachability is acceptable as it follows the osgi specification . And the WAB must has a unique context path to run, so WABs will not share the same servlet context.
TCCL is not stated in specification. Is it possible to set it to OsgiScopedServletContext classloader in whiteboard scenario?

[grgrzybek]

TCCL is not stated in specification. Is it possible to set it to servletContext classloader in whiteboard scenario?

But Whiteboard specification says that each filter/servlet should get own bundle's classloader, so in scenario like:

filter1 → filter2 → filter3 → servlet

each of these may get different servletContext.getClassLoader(). Of course I can wrap each filter/servlet again to specify the TCCL=servletContext.getClassLoader(), but I'd prefer leaving as-is.
Which means - if you need to access CL, use servletContext, not TCCL - especially because OSGi Core itself doesn't talk much (if at all) about TCCLs.

Do we both agree that TCCL should include the servlet bundle requested

yes

[…] but need not and should not include other servlet/filter bundles

not necessarily ;) My (at least for now) impression is that servlets/filters/listeners run in some environment, and the closest one is the containing "servlet context" and because this servlet context is almost like normal WAR/WebApplication, we shouldn't force separation of the bundles that contribute web elements into the given context...
For example, the TCCL includes pax-web-jsp bundle which provides JSP support and JSP servlet.

[qianwch]

For example, the TCCL includes pax-web-jsp bundle which provides JSP support and JSP servlet.

I thought that there is no jsp-support in whiteboard. And the specification states that ServletContext.getJspConfigDescriptor() returns null.
Is it possible to use jsp in whiteboard?

[grgrzybek]

I thought that there is no jsp-support in whiteboard. And the specification states that ServletContext.getJspConfigDescriptor() returns null.

Pax Web always supported JSP - it's kind of added value over the specification ;) (same as websockets, welcome files, SCIs, jetty handlers, ...) ;)

[qianwch]

I thought that there is no jsp-support in whiteboard. And the specification states that ServletContext.getJspConfigDescriptor() returns null.

Pax Web always supported JSP - it's kind of added value over the specification ;) (same as websockets, welcome files, SCIs, jetty handlers, ...) ;)

Yes, Pax Web supports JSP, but only for WAB(I have tested it), not in pax web http whiteboard.
I did not get jsp to work with whiteboard and found no examples about jsp in whiteboard.

At last, we do not need to support jsp in whiteboard, so set TCCL the same as servlet context cl could do no harm.
I have read some source code, I know the TCCL for http request processing Thread is set in Jetty/Tomcat/Undertow. Maybe we could change that in OsgiFilterChain.doFilter only for Whiteboard bundles.

[grgrzybek]

There's Whiteboard/Blueprint JSP configuration example here: https://github.com/ops4j/org.ops4j.pax.web/blob/web-8.0.9/samples/samples-whiteboard/whiteboard-blueprint/src/main/resources/OSGI-INF/blueprint/blueprint.xml#L33-L42

I know the TCCL for http request processing Thread is set in Jetty/Tomcat/Undertow.

Yes, but you can map this scenario to WAB case and TCCL there is correct.
For Whiteboard scenario, where most importantly you can mix servlets from different bundles in the same context (or single servlet into multiple contexts - with prototype scope), I'd really prefer leaving as-is.

If you really need to use ClassLoader at all in doGet() method, you could use the one from servletContext (according to Whiteboard specification) or setup a high-priority filter that'd set up TCCL for you for entire chain...

But still - if you have real world examples where TCCL should be setup for each step (instead of once) in F1 → F2 → F3 → … → Fn → Servlet chain, it's indeed a change in OsgiFilterChain, but I'd really prefer to avoid that.

[grgrzybek]

Anyway - new release is approaching, because I saw new Jetty versions today (9.4.49 and 10.0.12) - so we can think of something ;)

[qianwch]

But still - if you have real world examples where TCCL should be setup for each step (instead of once) in F1 → F2 → F3 → … → Fn → Servlet chain, it's indeed a change in OsgiFilterChain, but I'd really prefer to avoid that.

I am trying to use spring mvc+Thymeleaf in http whiteboard, thymeleaf-spring5 fails to run in NoClassDefFound Exception. It took me quite some time to location the problem is about TCCL. After I use a spring interceptor to set TCCL or apply your new fix, it could run now.
So there could be very few scenarios using F1->F2...->Servlet. But for whiteboard, we would not like to use the same TCCL for all bundles of one servlet context, as the servlet context cl is different.

TCCL is mentioned in servlet specification:

Anyway - new release is approaching, because I saw new Jetty versions today (9.4.49 and 10.0.12) - so we can think of something ;)

I suggest you could consider and discuss about the relation between TCCL and servlet context cl.

[grgrzybek]

yes, I mentioned chapter 10.7.2 in #1759 (comment)

But it's about Web Applications, so WABs ;) and it's fine there.

As you can see, I'm a bit hesitant. servletContext.getClassLoader() behavior is dictated by Whiteboard specification, so it's non-negotiable.
But for TCCL we have two options:

• as with current fix - an OSGi Servlet Context related class loader - delegating to all the bundles that contribute web elements to given context - that's how it works after currentThread.contextClassLoader does not include the Servlet bundle using http whiteboard. #1759 - and I understand that you don't want bundles of other servlets/filters for some servlet/filter
• set it to servletContext.getClassLoader() - the code is common to Whiteboard, HttpService and Whiteboard scenarios (you know - least surprise), but for WAB, servletContext.getClassLoader() == TCCL, so it's not a problem

How about ... giving you a configuration option?

[qianwch]

How about ... giving you a configuration option?

I think that's great! :)

[grgrzybek]

Give me some time - I'll check on Monday then.

[grgrzybek]

@qianwch I'm working on a fix. For now it'll be new PID property called org.ops4j.pax.web.tccl.type, which may take two values:

• servlet (the default, assumed value), which means current behavior, where ServletContext.getClassLoader() returns the classloader of a bundle for which given Servlet/Filter/Listener was registered, but TCCL is set to OsgiServletContextClassLoader which sees all the bundles of the web elements used for given ServletContext - and it's not the version you'd like to see, because it has to allow you to reach to other bundles - just like in normal WAR, classes from one JAR from /WEB-INF/lib should see / be able to load classes from another JAR from the same /WEB-INF/lib
• whiteboard - both ServletContext.getClassLoader() and TCCL will be set to the same classloader - bundle classloader for a given servlet/filter.

What do you think?

[grgrzybek]

@qianwch Please review 58f796c if that's what you need.

[qianwch]

@qianwch I'm working on a fix. For now it'll be new PID property called org.ops4j.pax.web.tccl.type, which may take two values:

• servlet (the default, assumed value), which means current behavior, where ServletContext.getClassLoader() returns the classloader of a bundle for which given Servlet/Filter/Listener was registered, but TCCL is set to OsgiServletContextClassLoader which sees all the bundles of the web elements used for given ServletContext - and it's not the version you'd like to see, because it has to allow you to reach to other bundles - just like in normal WAR, classes from one JAR from /WEB-INF/lib should see / be able to load classes from another JAR from the same /WEB-INF/lib
• whiteboard - both ServletContext.getClassLoader() and TCCL will be set to the same classloader - bundle classloader for a given servlet/filter.

What do you think?

Hi, I think that's great!

[grgrzybek]

The fix is pushed. Mind that there's no option for you to get a classloader reaching to more bundles without bundles of _other_servlets - you either get all with servlet option or only one bundle (so not the OsgiContextClassLoader) with whiteboard option for TCCL.