My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 280: daemon mode requires HttpServletRequest
  Back to list
Status:  Released
Owner:  code-rev...@gtempaccount.com
Closed:  Oct 2012


Sign in to add a comment
 
Reported by code-rev...@gtempaccount.com, Sep 24, 2009
Reported by Brad Larson <bklarson@gmail.com> on Fri Sep 18 13:44:12 PDT 2009
Source: JIRA GERRIT-281
Affected Version: 2.0.21

Per http://groups.google.com/group/repo-discuss/browse_thread/thread/3e657acdd894b402
the daemon mode requires HttpSevletRequest, which is wrong.  We should not
depend upon it.
Sep 24, 2009
#1 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Fri Sep 18 16:16:54 PDT 2009

I think I tracked this down to GerritGlobalModule binding
CanonicalWebUrlProvider, which in turn references a
Provider<HttpServletRequest>.  The bytecode references the type
HttpServletRequest, but is OK if the Provider<HttpServletRequest> is null.
Unfortunately the bytecode won't verify without the type present, even if we
are never going to use this code.

We need to rethink the way we build the GerritGlobalModule's Injector.  In non-
web mode we need to bind a provider that only knows about the config, but in
web mode we need to bind one that also knows about the HttpServletRequest.
Yuck.
Sep 24, 2009
#2 code-rev...@gtempaccount.com
Comment by Brad Larson <bklarson@gmail.com> on Fri Sep 18 16:41:11 PDT 2009

I know very little about Guice... what I did for the slave/master mode stuff
(for now) is to put conditional logic in the configure() method based on a
parameter passed to the constructor.  https://code.google.com/p/google-guice/wiki/AvoidConditionalLogicInModules

It is something they recommend to avoid, because it can lead to untested code,
but in this case it shouldn't be hard for us to guarantee that both paths get
tested.  I'm sure there are probably better methods though, and I'm not sure
if it would even apply to this situation.

Thanks for working on this... I'm still crawling up the Guice learning curve.
Sep 24, 2009
#3 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Fri Sep 18 17:28:08 PDT 2009

Yea, I'm trying to work up a different version where we change the bindings
based upon who is creating the Injector for the GerritGlobalModule.  Its the
more Guicey way to do something, but it is a bit more work.
Sep 24, 2009
#4 code-rev...@gtempaccount.com
Comment by Shawn Pearce <sop@google.com> on Fri Sep 18 17:47:15 PDT 2009

Fixed by Ibea06bfca8f6b2186ee2f6eff297d25318d6c418
Sep 24, 2009
#5 code-rev...@gtempaccount.com
Update by Shawn Pearce <sop@google.com> on Fri Sep 18 17:47:15 PDT 2009

Fixed in version 2.0.22.
Status: Fixed
Sep 25, 2009
#6 code-rev...@gtempaccount.com
(No comment was entered for this change.)
Labels: FixedIn-2.0.22
Oct 21, 2012
#7 sop@google.com
(No comment was entered for this change.)
Status: Released
Sign in to add a comment

Powered by Google Project Hosting