GuiceFilter uses wrong instance of FilterPipeline if used with multiple servlet context and multiple injectors

[gissuebot]

From carsten.schipke on June 07, 2011 21:57:53

In short:

Servlet Container: Jetty 8.0.0 M3 Guice: 3.0 + Servlet Extension (3.0)

If i create multiple servlet context and configure each of them with a dedicated injector based on a dedicated servlet module, GuiceFilter aquires always the FilterPipeline according to the last added servlet context/injector - no matter which servlet context my HTTP request leads to.

e.g.

Servlet Context A (context path: /a) Configured with dedicated GuiceFilter & GuiceServletContextListener providing Injector holding     Servlet Module incl. serve("/hello", HelloAServlet.class)

Servlet Context B (context path: /b) Configured with dedicated GuiceFilter & GuiceServletContextListener providing Injector holding     Servlet Module incl. serve("/hello", HelloBServlet.class)

If i add mentioned context in listed order, i will always receive HelloBServlet.class, no matter if i invoke /hello/a - or /hello/b in my browser.

I did not spend much time yet to find a clean solution, but what i can say is that some debugging in GuiceFilter tells me that it somehow uses the wrong (Managed)FilterPipeline on request / GuiceFilter.doFilter - but it is associated to the correct servlet context.

So if (i know, not clean solution) i extend GuiceFilter.setPipeline to put the here given FilterPipeline (which is still correct at this time) into provided servlet context AND then use this pipeline again in GuiceFilter.doFilter - all problems are solved.

Original issue: http://code.google.com/p/google-guice/issues/detail?id=635

[gissuebot]

From carsten.schipke on June 07, 2011 18:59:54

Of course i mean:

If i add mentioned context in listed order, i will always receive HelloBServlet.class, no matter if i invoke /a/hello - or /b/hello in my browser.

[gissuebot]

From cowwoc@bbs.darktech.org on June 18, 2011 19:10:41

I just ran into the exact same problem. See issue 637 . I think I've got an idea on how to fix this.

[gissuebot]

From cowwoc@bbs.darktech.org on June 18, 2011 20:20:02

I've attached a patch that fixes the problem (while maintaining backwards compatibility).

Attachment: gist    guice-filter.patch

[gissuebot]

From cowwoc@bbs.darktech.org on June 18, 2011 22:47:10

Please note that I am still experiencing race conditions with this patch, but far less frequently. Sometimes I see GuiceFilter.init() invoked without GuiceServletContextFilter and this leads to the original pipeline collision that was reported.

I suspect the problem is Grizzly-specific since it is the one instantiating GuiceFilter and GuiceServletContextFilter and I'm getting some exceptions with stack-traces that are purely Grizzly-related.

I need someone else to test the patch on their end, to isolate whether the problem is specific to the patch or specific to Grizzly.

[gissuebot]

From carsten.schipke on June 19, 2011 03:52:30

Sorry, i did not have had the time yet to further take care for this issue. Thank you for your response / sharing the problem. I will definitly have a look at your patch and give you feedback during next week.

As i mentioned in my original post, i already have a kind of hotfix which i am currently using to proceed with development. I did not experience further problems with this solution yet. Putting the correct FilterPipeline into correct ServletContext seems to be fine. I could not spend more time and search the problem's real source and thereby find its real solution, yet.

Anyway, main reason for this ticket for me is that i hope for some official feedback. Since my project is business relevant, i do not really want to move the issue to a later time by patching 3rd party libraries.

I would rather like to get some information about:

- If we can hope for an official update/release that solves this problem - and if yes, when could this happen?

• If there is a workaround without touching the libraries?
• If it is just my/our fault and what i want is already working fine.

[gissuebot]

From mcculls on June 20, 2011 04:19:50

FYI, I have a similar (but smaller) patch in issue 618 which basically exposes the FilterPipeline API so that third-party extensions can implement their own delegation strategies. We're successfully using this to provide servlets from multiple injectors: https://github.com/sonatype/nexus/blob/guice-servlet/nexus/nexus-web-utils/src/main/java/org/sonatype/nexus/web/NexusGuiceFilter.java

[gissuebot]

From mcculls on June 20, 2011 04:23:38

Issue 637 has been merged into this issue.

[gissuebot]

From cowwoc@bbs.darktech.org on June 20, 2011 05:19:59

mcculls,

In the case of issue 618 , who would invoke setInjectedPipeline()? As far as I can tell, I have no way of accessing GuiceFilter from GuiceServletContextListener in order to invoke setInjectedPipeline().

[gissuebot]

From mcculls on June 20, 2011 05:39:53

It's visible from any subclass of GuiceFilter, so you just need to extend GuiceFilter with your desired strategy for locating pipelines and use that subclass in your web.xml (or wherever you configure the filter-class). Note that the static functionality in the original base class is unchanged, all the subclass affects is the managed/injected functionality.

Note: the patch in issue 618 may well not be the best solution, it's just the simplest solution I've found so far.

[gissuebot]

From cowwoc@bbs.darktech.org on June 20, 2011 06:31:05

mcculls,

The key point is "with your desired strategy for locating pipelines". In my case, once the Injector is created by GuiceServletContextListener I'd like to set the pipeline to injector.getInstance(FilterPipeline.class). Unfortunately, I don't have access to the GuiceFilter instance associated with the current server.

My patch moves this code into GuiceFilter.init() simply because that's the only way I found for linking GuiceFilter and GuiceServletContextListener... Now that I think about it, I guess I could still implement the same thing using your patch simply by overriding Filter.init() in my GuiceFilter subclass.

One final point: I don't understand why GuiceFilter and GuiceServletContextListener need to be separate entities. If you're going to require users to subclass GuiceFilter why not simply merge the two? Asking users to introduce two separate classes into web.xml in order to take advantage of Guice is accident-prone. We're better off having one class (especially since the two need to share data).

[gissuebot]

From mcculls on June 20, 2011 07:26:48

> The key point is "with your desired strategy for locating pipelines". In my case, once the Injector is created > by GuiceServletContextListener I'd like to set the pipeline to injector.getInstance(FilterPipeline.class).

Wouldn't you want to use a delegating approach, so you can handle multiple pipelines at the same time?

> Unfortunately, I don't have access to the GuiceFilter instance associated with the current server.

Yes, we have control over both the Filter and ServletContextListener(s) in our prototype application, so we can therefore tell the Filter about new injectors. We use static injection to inject a list of pipelines - this list is then updated as servlet injectors are created. Our servlets share the same basic config, just like how Guice servlets extend the same base servlet module, and this is how we "call-home" as each injector is initialized. Then as requests come in we simply iterate over the pipeline sequence looking for one that can handle the request.

> My patch moves this code into GuiceFilter.init() simply because that's the only way I found for linking > GuiceFilter and GuiceServletContextListener... Now that I think about it, I guess I could still implement > the same thing using your patch simply by overriding Filter.init() in my GuiceFilter subclass.

Exactly, if you extend GuiceFilter (plus the patch in Issue 618 ) then you can choose how they're linked - personally I'd use some form of injection to loosen the coupling between the filter and context listener.

> One final point: I don't understand why GuiceFilter and GuiceServletContextListener need to be separate > entities. If you're going to require users to subclass GuiceFilter why not simply merge the two? Asking > users to introduce two separate classes into web.xml in order to take advantage of Guice is accident-prone. > We're better off having one class (especially since the two need to share data).

I assume it's separation of concerns - GuiceFilter is responsible for filtering and dispatching requests, whereas GuiceServletContextListener is responsible for setting up the injector(s). This makes sense to me, and in fact it wouldn't make sense to combine these two classes for our particular application. Besides, afaict most people shouldn't have to extend GuiceFilter, and those that do need to extend it should know enough to be able to manage the two classes.

Anyway, this is just what works for us (I prefer small patches) - it may not suit your particular application.

[gissuebot]

From cowwoc@bbs.darktech.org on June 20, 2011 08:50:39

mcculls,

> Wouldn't you want to use a delegating approach, so you can handle multiple pipelines at the same time?

What does a delegating approach really mean in this case? I want to run multiple unit tests in parallel where each @Test method gets a separate Injector and web server to run against. A delegating approach sounds like we're saying: if server 1 cannot handle a request, server 2 should try to, then server 3 and so on. That sounds more like fail-over to me and doesn't really make sense for my unit test use-case.

> Besides, afaict most people shouldn't have to extend GuiceFilter, and those that do need to extend it should know enough to be able to manage the two classes.

Agreed, which is why I'd prefer adding code to GuiceFilter that automatically tries grabbing a FilterPipeline from GuiceServletContextListener at init() time. If I understand your use-case correctly, you want to be able to modify the filter pipeline multiple times over the lifetime of the Filter (whereas I only want it set once). What we can do is have init() pick up reasonable defaults and add the setter method you proposed. This way both our use-cases are possible.

[gissuebot]

From mcculls on June 20, 2011 10:46:45

> > Wouldn't you want to use a delegating approach, so you can handle multiple pipelines at the same time? > > What does a delegating approach really mean in this case? I want to run multiple unit tests in > parallel where each @Test method gets a separate Injector and web server to run against.

Usually I'd run that sort of test (ie. more integration test than unit test) in an isolated classloader or jvm to avoid static clashes elsewhere in the app or container, but I can see what you're trying to achieve.

This issue covers many use-cases, and while a last-one-wins approach is fine for your test use-case it won't work for our situation where we want to manage multiple injectors concurrently under a single filter instance. For example, injector A maps /a -> servletA while at the same time injector B maps /b -> servletB.

> A delegating approach sounds like we're saying: if server 1 cannot handle a request, server 2 > should try to, then server 3 and so on. That sounds more like fail-over to me and doesn't really > make sense for my unit test use-case.

Our use-case is a plugin system, with one injector per-plugin running inside the same servlet container. Each plugin can contribute servlet bindings, so we have a central delegating filter that checks each active pipeline to see whether it can handle the incoming request.

> > Besides, afaict most people shouldn't have to extend GuiceFilter, and those that do need to > > extend it should know enough to be able to manage the two classes. > > Agreed, which is why I'd prefer adding code to GuiceFilter that automatically tries grabbing a > FilterPipeline from GuiceServletContextListener at init() time. If I understand your use-case > correctly, you want to be able to modify the filter pipeline multiple times over the lifetime of > the Filter (whereas I only want it set once). What we can do is have init() pick up reasonable > defaults and add the setter method you proposed. This way both our use-cases are possible.

Sure, you could easily build a single solution on top of the patch from issue 618 .

[gissuebot]

From cowwoc@bbs.darktech.org on June 20, 2011 11:14:34

mcculls,

> Usually I'd run that sort of test (ie. more integration test than unit test) in an isolated classloader or jvm to avoid static clashes elsewhere in the app or container, but I can see what you're trying to achieve.

That's actually a very good idea though I have a feeling I'm going to have a hard time convincing the Surefire committers to add such a feature. Doing this myself on a per-@Test basis sounds painful.

I'm fine with using your patch so long as:

1. Currently MULTIPLE_INJECTORS_WARNING is logged if the static pipeline value is clobbered. I propose logging this warning only if the clobbered value actually gets used (this is included in my patch). Otherwise we're logging warnings that aren't actually relevant.
2. We document how to run multiple instances of GuiceFilter per JVM (my use-case). If subclassing is required, so be it, so long as we post the relevant code-sniplet somewhere.

[gissuebot]

From cowwoc@bbs.darktech.org on June 20, 2011 11:25:23

Filed http://jira.codehaus.org/browse/SUREFIRE-749

[gissuebot]

From carsten.schipke on June 20, 2011 19:47:03

Just to make this clear - as far as i understand, the current guice release is supposed to support multiple injectors?

So should this include above mentioned use case and reason for this ticket - and will there be a fix/change? Or is discussed solution which by-passes the problem by using extended visibility of implementation details the way to go?

Wouldn't this be a massive break into the 'guice api'? Using this patch & extending visibility means i rely on implementation details that might change in future.. Besides other possible problems (e.g. in concurrent context) i am not able to foresee now.

Anyway, i also think it would be better to have more specific documentation on how to implement required use case, if there wont be an official way/fix/change ...

[gissuebot]

From cowwoc@bbs.darktech.org on June 21, 2011 05:36:45

Having given this some more thought, I believe using different ClassLoaders is more of a band-aid than a proper solution. One of the main selling points of Guice was that by migrating away from static factories we'd improve unit test isolation. This is precisely the problem we are encountering here.

[gissuebot]

From cowwoc@bbs.darktech.org on June 21, 2011 05:49:03

mcculls,

Keeping separation of concerns in mind, can you please explain what GuiceFilter and GuiceServletContextListener are meant to do, independently of one another?

[gissuebot]

From mcculls on June 21, 2011 05:50:45

> Having given this some more thought, I believe using different ClassLoaders is more of a band-aid than a > proper solution. One of the main selling points of Guice was that by migrating away from static factories > we'd improve unit test isolation. This is precisely the problem we are encountering here.

I didn't say Guice shouldn't support this use-case (it's entirely possible with either patch) - I was making the point that your tests could always run into issues with other third-party libraries that rely on static members, because those tests are spinning up live servlet containers, etc. and not AFAIK performing any mocking.

So while Guice can certainly help you move away from static factories, it can't magically replace them in existing code - which is where mocking can help, because you then don't rely on having a live container.

Anyway, back to the original issue - it would be good to get Dhanji's view on this...

[gissuebot]

From mcculls on June 21, 2011 06:07:09

> Keeping separation of concerns in mind, can you please explain what GuiceFilter > and GuiceServletContextListener are meant to do, independently of one another?

I believe I answered this in an earlier comment (note: its just my personal view, I didn't design these classes)

* GuiceFilter is responsible for filtering requests/responses by using the pipeline(s) injected into it

• GuiceServletContextListener is responsible for creating injectors on-demand that inject pipelines

So while there is a link between the two - one creates injectors, the other expects pipeline(s) to be injected - they could conceivably work independently, in that you could put in your own Filter implementation that just used the injector created and stashed in the context by the listener. Or you could write your own listener that used a different pipeline implementation (assuming the FilterPipeline type was opened up as in my patch).

Conversely, why should these classes be combined? They implement different interfaces that have no relation or reference to each other in the servlet spec, so combining them is imho purely an implementation detail to avoid a little bit of indirection.

[gissuebot]

From cowwoc@bbs.darktech.org on June 21, 2011 06:55:18

mcculls,

I think GuiceFilter has two separate roles:

1. Provide an Injector to registered servlets/filters. Meaning, the servlet API instantiates GuiceFilter and it injects any registered servlets/filters. The emphasis here is that without GuiceFilter we wouldn't have access to an Injector.
2. Replace web.xml. Convert your configuration into type-safe code.

Assuming I understand your use-case correctly, I argue that GuiceFilter doesn't need to serve different pipelines (more on this below).

Keeping #1 in mind, I'd advocate merging GuiceFilter and GuiceServletContextListener (such that each GuiceFilter instance would be bound to exactly one Injector). Users who wish to specify multiple injectors could simply specify multiple GuiceFilter instances (one per Injector). The Servlet API's existing filter-chain mechanism would traverse one GuiceFilter/injector after another.

The merged class would still enable you to move your configuration from web.xml into code. Instead of having one entry per GuiceServletContextListener subclass in web.xml, you'd have one line per GuiceFilter with its own Injector.

Would you be able to implement your use-case in terms of multiple GuiceFilter subclasses, as mentioned above?

[gissuebot]

From DavidBild on January 31, 2012 22:49:00

This issue really needs to be fixed. As comment 5 on issue 618 states, with the October 2011 changes, making FilterPipeline public and the GuiceFilter(FilterPipeline) constructor visible (ideally public, but protected is workable as well) is sufficient.

The JVM-scope staticness (actually, classloader-scope staticness) of GuiceFilter is a real blight on the library.  The code is in place to not rely on it accept for backwards compatibility (i.e., dynamic filterpipelines are supported; the constructor is just not visible). Please make it visible.

[gissuebot]

From cowwoc@bbs.darktech.org on September 22, 2012 23:23:04

It occurs to me that I never stated my use-case, so here it is:

I am attempting to run unit tests concurrently. Each @Test runs against its own web server. Each web server instantiates its own Injector and (as a consequence) its own GuiceFilter instance. The problem is that all instances share the GuiceFilter's static fields.

The goal is two-fold:

1. Allow GuiceFilter to be used in a thread-safe manner.
2. Remove spurious thread-safety warnings (the one about multiple injectors).

Reducing the logging level is not the same as removing spurious warnings. If there is no danger, please don't log at all!

How you solve this problem is entirely up to you, but please fix this soon. Let me know if there is anything I can do to help.

[gissuebot]

From mcculls on September 24, 2012 03:56:42

Could you perhaps provide a JUnit based testcase? (See the current tests under extensions/servlet.)

That would a) help verify any potential solution and b) provide others with a multi-context example.

[gissuebot]

From cowwoc@bbs.darktech.org on September 24, 2012 22:09:56

I'm not sure how to write a JUnit test that verifies the two goals I mentioned. I don't think unit tests are the right way to validate the thread-safety of a class. Nor am I sure how to check whether a particular warning message was logged in the course of an HTTP request.

Why are you asking for a JUnit testcase? Is there any ambiguity in the stated goals or in how to reproduce the spurious warnings?

[gissuebot]

From sameb@google.com on September 25, 2012 06:37:13

Tests (junit-based or not) help to ensure the changes fix exactly what is requested, and that future maintenance on the code doesn't cause regressions.  It's standard practice to want a test confirming every change.

[gissuebot]

From vaclavslovacek on October 11, 2012 03:33:38

Are there any updates on this issue? This makes practically impossible to use Guice for multiple Contexts without having to modify it. It even prompted me to have a look on released versions of Guice and it does almost seem that Guice is a dead project. This isuues is quite critical for some use cases and not fixed for over a year :(

[gissuebot]

From ohadshai on November 11, 2013 08:35:27

Is there any chance it will be fixed?

[gissuebot]

From dhanji on May 20, 2014 20:52:57

Are people still using multiple contexts? Doesn't classloader isolation in say, Tomcat, solve this?

[gissuebot]

From cowwoc@bbs.darktech.org on May 21, 2014 05:08:06

The problem isn't multiple contexts. The problem is unit tests, where you might have multiple servers, each instantiating their own GuiceFilter, but they are all running inside the same Classloader.

I tried asking both JUnit and TestNG authors to add Classloader isolation and they both refused claiming this would degrade performance and cause annoying casting problems.

So, here we are.

[gissuebot]

From SirReto on May 21, 2014 05:23:20

I have a valid use case, where this issue proved to be troublesome. I applied the current patch presented here and it worked. Having applications A and B, both packed as EAR having EJB's. Both have Guice DI, provided by GWTP.

Both applications have it's own instances of the GuiceFilter. I tried adding A's EJBs classes to the /lib folder of the the B application, but invoking the EJB proved to be unsuccessful, because of the classloading issues. FooEJB from A project wasn't the same as the FooEJB at the B project /lib folder.

I removed the A application from the /lib folder of the B project and deployed as a dependent module (on JBoss 7.20). Now the B project shares the EJB classes from A project, but also the GuiceContext. Issues when I tried load a A application URL and B GuiceFilter would "intercept" and try to process it. EJB worked fine. Refactoring A application common classes would work, but that would be too much work (big blob of legacy code), patching was simpler. Patched worked on Guice 4.

[gissuebot]

From dhanji on May 21, 2014 16:22:54

@cowwoc I see, that's a good point re: tests. The static in GuiceFilter is quite annoying I agree. However, I want to solve this without breaking the ease of use of in servlet environments where web.xml instantiates filters directly.

Let me think about this issue a bit. Im loathe to expose internals as issue #618 suggests unless absolutely necessary.

[gissuebot]

From cowwoc@bbs.darktech.org on May 21, 2014 16:27:03

Remind me again, why do we need a static variable for web.xml instantiated filters? I've been months/years since I looked at this code, so I forget the details.

[gissuebot]

From dhanji on May 27, 2014 18:16:23

That's legacy--I'm trying to see if there's a way to extricate ourselves from that mess, but a lot of clients depend on the broken behavior so it's not trivial.