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

[GoogleCodeExporter]

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 reported on code.google.com by carsten....@gmail.com on 8 Jun 2011 at 1:57

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

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.

Original comment by carsten....@gmail.com on 8 Jun 2011 at 1:59

[GoogleCodeExporter]

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

Original comment by cow...@bbs.darktech.org on 19 Jun 2011 at 2:10

[GoogleCodeExporter]

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

Original comment by cow...@bbs.darktech.org on 19 Jun 2011 at 3:20

Attachments:

• guice-filter.patch

[GoogleCodeExporter]

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.

Original comment by cow...@bbs.darktech.org on 19 Jun 2011 at 5:47

[GoogleCodeExporter]

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.

Original comment by carsten....@gmail.com on 19 Jun 2011 at 10:52

[GoogleCodeExporter]

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/m
ain/java/org/sonatype/nexus/web/NexusGuiceFilter.java

Original comment by mccu...@gmail.com on 20 Jun 2011 at 11:19

[GoogleCodeExporter]

Issue 637 has been merged into this issue.

Original comment by mccu...@gmail.com on 20 Jun 2011 at 11:23

[GoogleCodeExporter]

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().

Original comment by cow...@bbs.darktech.org on 20 Jun 2011 at 12:19

[GoogleCodeExporter]

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.

Original comment by mccu...@gmail.com on 20 Jun 2011 at 12:39

[GoogleCodeExporter]

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).

Original comment by cow...@bbs.darktech.org on 20 Jun 2011 at 1:31

[GoogleCodeExporter]

> 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.

Original comment by mccu...@gmail.com on 20 Jun 2011 at 2:26

[GoogleCodeExporter]

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.

Original comment by cow...@bbs.darktech.org on 20 Jun 2011 at 3:50

[GoogleCodeExporter]

> > 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.

Original comment by mccu...@gmail.com on 20 Jun 2011 at 5:46

[GoogleCodeExporter]

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.

Original comment by cow...@bbs.darktech.org on 20 Jun 2011 at 6:14

[GoogleCodeExporter]

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

Original comment by cow...@bbs.darktech.org on 20 Jun 2011 at 6:25

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

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 
...

Original comment by carsten....@gmail.com on 21 Jun 2011 at 2:47

[GoogleCodeExporter]

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.

Original comment by cow...@bbs.darktech.org on 21 Jun 2011 at 12:36

[GoogleCodeExporter]

mcculls,

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

Original comment by cow...@bbs.darktech.org on 21 Jun 2011 at 12:49

[GoogleCodeExporter]

> 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...

Original comment by mccu...@gmail.com on 21 Jun 2011 at 12:50

[GoogleCodeExporter]

> 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.

Original comment by mccu...@gmail.com on 21 Jun 2011 at 1:07

[GoogleCodeExporter]

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?

Original comment by cow...@bbs.darktech.org on 21 Jun 2011 at 1:55

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

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.

Original comment by DavidB...@gmail.com on 1 Feb 2012 at 6:49

[GoogleCodeExporter]

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.

Original comment by cow...@bbs.darktech.org on 23 Sep 2012 at 6:23

[GoogleCodeExporter]

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.

Original comment by mccu...@gmail.com on 24 Sep 2012 at 10:56

[GoogleCodeExporter]

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?

Original comment by cow...@bbs.darktech.org on 25 Sep 2012 at 5:09

[GoogleCodeExporter]

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. 

Original comment by sa...@google.com on 25 Sep 2012 at 1:37

[GoogleCodeExporter]

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 :(

Original comment by vaclavsl...@gmail.com on 11 Oct 2012 at 10:33

[GoogleCodeExporter]

Is there any chance it will be fixed?

Original comment by ohads...@gmail.com on 11 Nov 2013 at 4:35

[GoogleCodeExporter]

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

Original comment by dha...@gmail.com on 21 May 2014 at 3:52

[GoogleCodeExporter]

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.

Original comment by cow...@bbs.darktech.org on 21 May 2014 at 12:08

[GoogleCodeExporter]

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. 

Original comment by SirR...@gmail.com on 21 May 2014 at 12:23

[GoogleCodeExporter]

@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.

Original comment by dha...@gmail.com on 21 May 2014 at 11:22

[GoogleCodeExporter]

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.

Original comment by cow...@bbs.darktech.org on 21 May 2014 at 11:27

[GoogleCodeExporter]

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.

Original comment by dha...@gmail.com on 28 May 2014 at 1:16