xiaodududu / google-guice

Automatically exported from code.google.com/p/google-guice
Apache License 2.0
0 stars 0 forks source link

Cannot suppress GuiceFilter warning #636

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
I'm attempting to run concurrent unit tests (each one with their own Injector, 
web server, etc) so I'd like to suppress the warning message emitted by 
GuiceFilter.setPipeline. setPipeline() invokes:
       Logger.getLogger(GuiceFilter.class.getName()).warning(MULTIPLE_INJECTORS_WARNING);

java.util.logging.Logger.getLogger() warns "The LogManager may only retain a 
weak reference to the newly created Logger. It is important to understand that 
a previously created Logger with the given name may be garbage collected at any 
time if there is no strong reference to the Logger."

Because GuiceFilter does not retain a strong reference to the Logger, it gets 
garbage-collected between the time I configure it and the time that 
GuiceFilter.setPipeline() gets invoked.

Normally the class containing the log is supposed to retain a static reference 
to the Logger, precisely for this reason. I am configuring the logging level 
from an xml configuration file. If I am forced to store a strong reference to 
the Logger on behalf of GuiceFilter it's not entirely clear where I should 
store such a reference there are multiple unit test files and order of 
execution is not guaranteed.

Please consider adding a static reference to the Logger.

Also, it's not entirely clear the impact of running concurrent unit tests with 
GuiceFilter. I see multiple locations GuiceFilter.pipeline is read and written 
to. Are you *sure* this won't result in threading problems when running 
multiple servers handling concurrent tests?

Original issue reported on code.google.com by cow...@bbs.darktech.org on 17 Jun 2011 at 2:14

GoogleCodeExporter commented 9 years ago
So basically:

1) you want a static reference to the GuiceFilter logger
2) you are asking about concurrency issues without providing any details

Please confirm.

Fred

Original comment by ffa...@gmail.com on 17 Jun 2011 at 8:27

GoogleCodeExporter commented 9 years ago
1. Yes.
2. I'm asking what's the impact of this scenario:

Thread1: invoke Guice.createInjector() which invokes GuiceFilter.setPipeline() 
indirectly.
Thread1: GuiceFilter.init() invoked, GuiceFilter.pipeline gets read

Thread2: invoke Guice.createInjector() which invokes GuiceFilter.setPipeline() 
indirectly.
Thread2: GuiceFilter.init() invoked, GuiceFilter.pipeline gets read

Thread1: invoke doFilter(), which reads Guice.pipeline set by Thread2
Thread1: invoke destroy(), which sets GuiceFilter.pipeline to 
DefaultFilterPipeline()

Thread2: invoke doFilter(), which reads Guice.pipeline set by Thread1
etc...

Is there a problem with the fact that Threads are using pipelines set by other 
threads?

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

GoogleCodeExporter commented 9 years ago
Thanks for the details.

The filter pipeline was retrofitted in 
http://code.google.com/p/google-guice/source/detail?spec=svn1150&r=1150 to 
accommodate the multiple injector case.  This means your concurrent tests 
should run just fine.

As far as where to put the logger handle, I don't see an issue with adding a 
static handle in GuiceFilter.  You could probably do this in your suite as 
well, but I agree that it's probably best done in the more conventional 
location.

Original comment by ffa...@gmail.com on 17 Jun 2011 at 6:15

GoogleCodeExporter commented 9 years ago
Okay, so let's just add the static Logger and I'll open a separate issue if I 
run into any race conditions.

Original comment by cow...@bbs.darktech.org on 17 Jun 2011 at 6:38

GoogleCodeExporter commented 9 years ago
fixed the remaining issue in r1587.

Original comment by sberlin on 16 Oct 2011 at 3:56