Open GoogleCodeExporter opened 9 years ago
[deleted comment]
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
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
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:
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
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
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
Issue 637 has been merged into this issue.
Original comment by mccu...@gmail.com
on 20 Jun 2011 at 11:23
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
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
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
> 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
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
> > 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
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
Filed http://jira.codehaus.org/browse/SUREFIRE-749
Original comment by cow...@bbs.darktech.org
on 20 Jun 2011 at 6:25
[deleted comment]
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
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
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
> 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
> 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
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
[deleted comment]
[deleted comment]
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
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
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
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
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
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
Is there any chance it will be fixed?
Original comment by ohads...@gmail.com
on 11 Nov 2013 at 4:35
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
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
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
@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
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
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
Original issue reported on code.google.com by
carsten....@gmail.com
on 8 Jun 2011 at 1:57