New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Are Http scopes and scope implementations sufficiently general? #100

Closed
gissuebot opened this Issue Jul 7, 2014 · 19 comments

Comments

Projects
None yet
1 participant
@gissuebot
Copy link

gissuebot commented Jul 7, 2014

From cquezel on May 03, 2007 21:49:09

Are Http scopes and scope implementations sufficiently general?

I am very new to Guice so what I am proposing here may not be correct ...
but here it goes.

I was looking at the scope features (doc please ...) to figure how I could
use scopes in my applications. I noticed that the REQUEST and SESSION
scopes are very http Servlet oriented. I was wondering if this was
necessary.

I think that non http Servlet based application could benefit from general
purpose REQUEST and SESSION scopes. Http Servlet based application could
also benefit from this. So I am proposing create more general REQUEST and
SESSION scopes and to use these to implement http Servlet scoping.

First, I would define a general purpose class for thread based scope to
take over the non http Servlet API part of the GuiceFilter class. For
example:

public final class RequestScopeSetter {

    private RequestScopeSetter()
    {
        // Hide
    }
    
    private static final ThreadLocal<Map<String, Object>> REQUEST_CACHE
        = new ThreadLocal<Map<String, Object>>();

    public static void beginScope()
    {
        REQUEST_CACHE.set(new HashMap<String, Object>());
    }

    public static void endScope()
    {
        REQUEST_CACHE.set(null);
    }
    
    static Map<String, Object> getCache() {
        Map<String, Object> cache = REQUEST_CACHE.get();
        if (cache == null) {
            throw new IllegalStateException(
                "Cannot access request scoped object. " +
                "Either we are not currently inside a " +
                "scoped thread, or you may have forgotten " +
                "to initialize the thread with " +
                RequestScopeSetter.class.getName() +
                ".beginScope() method."
            );
        }
        return cache;
    }
}

Next I would modify the Filter class to use this mechanism (or define
another Filter):

  public void doFilter(ServletRequest servletRequest,
      ServletResponse servletResponse, FilterChain filterChain)
      throws IOException, ServletException {
    Context previous = localContext.get();
    try {
      localContext.set(new Context((HttpServletRequest) servletRequest,
          (HttpServletResponse) servletResponse));
      RequestScopeSetter.beginScope();
      try {
          filterChain.doFilter(servletRequest, servletResponse);
      } finally {
        RequestScopeSetter.endScope();
      }
    
    } finally {
      localContext.set(previous);
    }
  }

I would maybe define a Runnable for other uses:

class RequestScopeSetterRunnable implements Runnable {
    private final Runnable toRun;
    RequestScopeSetterRunnable(Runnable toRun)
    {
        this.toRun = toRun;
    }
    
    public void run()
    {
        RequestScopeSetter.beginScope();
        try {
            toRun.run();
        } finally {
            RequestScopeSetter.endScope();
        }
    }
}

The REQUEST scope would be implemented as:

final class Scopes {
    
    public static final Scope REQUEST = new Scope() {
        public <T> Provider<T> scope(Key<T> key,
            final Provider<T> creator) {
            final String name = key.toString();
            return new Provider<T>() {
                public T get() {
                    Map<String, Object> threadCache
                        = ThreadScopeSetter.getCache();
                    synchronized (threadCache) {
                        @SuppressWarnings("unchecked")
                        T t = (T) threadCache.get(name);
                &nbs...

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

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From kevinb9n on May 15, 2007 09:57:25

I believe we may have made a mistake in locating the @RequestScoped and
@SessionScoped annotations in com.google.inject.servlet.  The concepts they represent
should not have been tied to the servlet concept.  Our ServletScopes implementations
for these are servlet-specific, of course.  But this would let you bind those same
annotations to whatever you please without having to import from
com.google.inject.servlet.

Still, I don't know what we can do about this now.

Status: Accepted
Owner: kevinb9n

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From kevinb9n on June 03, 2007 11:34:38

I suppose the best we can do is fix the documentation on those two annotations and
move them to the main jar file.  But we can't change the package. :(

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From crazyboblee on June 03, 2007 11:41:51

Do we really need "request" and "session" scope outside of a web application?

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From kevinb9n on June 03, 2007 11:48:05

Why wouldn't we?  Perhaps it just has an RMI or other RPC interface.  And perhaps it
wants to reuse modules that are also used by a web front-end.  It sounds totally
plausible to me.  How or why they'd use Session is a mite questionable, of course.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From crazyboblee on June 03, 2007 12:00:37

When I said "really," I meant "really," as in does somebody really need this badly
enough to futz up the API? Because if they do, we will. I'm not sure "request" is the
right word for RPC calls anyway.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From cquezel on June 05, 2007 10:56:25

I personally need request and thread scopes to be the same. I don't have an issue
with session (I included it for generality only). Our web application (probably
others also) has a scheduler to execute tasks. Much of the code used in tasks is
common to http requests. I.e. there is not much difference between request performing
certain actions and a scheduler thread performing other actions. In fact most of the
code is Servlet unaware.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 06, 2007 12:49:29

+1 for the addition of a thread scope. I also created my own, so that makes two...
I use it to scope objects in a per request security context. To make my code work in
a standalone scenario, I can't live without that thread scope. I'll take a look if I
can donate some of my code, but it obviously looks similar to cquezel's.

I wouldn't bother with session scope either.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 07, 2007 04:40:32

My code attached, including simple unit test. Feedback appreciated.

Attachment: gist
   GuiceThreadScope.txt

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 07, 2007 05:37:02

Blogged about it (includes code example): http://garbagecollected.wordpress.com/2007/06/07/guice-thread-scope/

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From feng.ronald on June 07, 2007 05:45:19

Guice is very flexible, you can add any scope you want.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From kevinb9n on June 07, 2007 09:28:30

Wait a minute -- you say "thread scope", but then you say "per request".  Which is
it?  Is it a request scope, or a thread scope?  A "thread scope" would be something
that lives for the entire lifetime of the thread.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 07, 2007 10:09:29

It could be both, actually. If you don't reset it, it lives as long as the thread,
and if you do, it could live as long as a request. Pure thread scoping probably
isn't always what you want if you do thread pooling (or if your ejb/web/whatever
container does). So the scope is thread, or less. Does that make sense?

Maybe the upcoming conversation scope addresses some of this? (the "or less" part?)

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From kevinb9n on June 07, 2007 10:34:06

I don't think there's any value in the concept of "resetting" a scope.  If you want
it to live as long as the thread does (e.g. you just want to reuse SimpleDateFormat
instances), that's thread scope; if you want the lifetime to be shorter, you want
your own Request or Job or Task or UnitOfWork or Foo scope.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 07, 2007 11:41:54

Thanks Kevin: you're right. What I implemented is a UnitOfWork scope that, in my
specific scenario, is able to replace the HTTP Request scope. And if you don't use
the reset() method in my code, it will behave exactly like a thread scope.

I gave all this some more thought, and I agree that it doesn't make sense to add an
HTTP Request scope replacement to Guice. What we need is a Conversation scope that
isn't web specific. How will you guys implement Conversation scope?

Thread scope could still be useful, and perhaps we should raise a new issue for that
one.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 08, 2007 03:27:50

Created issue #100 for the Thread scope thing.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 08, 2007 03:28:53

Argh.. issue #114 , I mean.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 11, 2007 07:50:08

I've been able to get rid of my use case for this issue, after changing my
application to use pure Thread scope as implemented in issue #114 .

First, I define an interface like this:

public interface GuiceCallable<V> {
    public V call(Injector injector);
}

And then I use a template class that creates a thread per method invocation:

public class GuiceTemplate {
    ...
    public <V> V call(final GuiceCallable<V> c) {
        Future<V> future = Executors.newSingleThreadExecutor().submit(
            new Callable<V>() {
                public V call() {
                    return c.call(this.injector);
                }
            }
        );
        try {
            return future.get();
        } catch (InterruptedException e) {
            ...
        } catch (ExecutionException e) {
            ...
        }
    }
}

So I guess this is an extra use case for issue #114 .

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From robbie.vanbrabant on June 11, 2007 15:04:22

I think this issue can be closed.

My summary:

  • HTTP Request scope can be replaced by using THREAD scope. The only caveat I spot
    is when one creates threads within the template; people who need that could
    implement their own THREAD scope with InheritableThreadLocal, or create a custom
    solution.
  • An HTTP Session scope replacement is probably less important. For a lot of
    standalone (as in Swing, ...) applications, SINGLETON will do just fine because only
    one application instance will run at a time in a single JVM.
  • As for the Conversation scope for non web apps (which I mentioned), it will be
    hard to implement a solution that is generic enough for most people to use. But it
    would be really, really cool.

Migration from HTTP to THREAD/... can be as easy as replacing some bindings, and
perhaps rewriting one class. That's not too bad.

@gissuebot

This comment has been minimized.

Copy link

gissuebot commented Jul 7, 2014

From limpbizkit on May 31, 2008 12:23:44

Robbie summarized this nicely. We're going to leave everything as-is. Issue 114 will cover thread scope,
inheritable thread scope etc., and whether they should be included.

I think the best takeaway on this issue is that we should write a how-to doc that walks through implementing a
custom scope.

Status: WontFix

@gissuebot gissuebot closed this Jul 7, 2014

leusonmario pushed a commit to leusonmario/guice that referenced this issue Aug 16, 2018

Merge pull request google#100 from hazendaz/master
Update maven wrapper and mybatis parent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment