What's wrong with my ThreadLocal scope implementation?

[Rahul]

Something is amiss with the ThreadLocal scope implementation below.
Can someone please explain what am I missing? I am seeing the Scope
applied only the first time I request the instance of a Entity
decorated entity.

Thanks in advance,
Rahul

-------------- code snippet below -------------

public class ThreadLocalScope implements Scope {

public static final ThreadLocal<Provider<?>> threadlocal = new
ThreadLocal<Provider<?>>();

@Override
public <T> Provider<T> scope(Key<T> key, final Provider<T> unscoped)
{
System.out.println("Scoping " + key.toString());
Provider<T> retVal = null;
if (null != threadlocal.get()) {
retVal = (Provider<T>) threadlocal.get();
} else {
retVal = new Provider<T>() {

@Override
public T get() {
return unscoped.get();
}

};
threadlocal.set(retVal);
}
return retVal;
}

}

[Logan Johnson]

I think that's exactly when scopes are applied I think what you want to do is always return a Provider which in turn wraps your ThreadLocal.

public static final ThreadLocal<?> threadlocal = new

ThreadLocal<?>();

public <T> Provider<T> scope(Key<T> key, final Provider<T> unscoped) {

   return new Provider<T> {

public T get() {

if (null != threadlocal.get()) {

    return threadlocal.get();

   } else {

      // ...

    }

}

}

}

Incidentally, your ThreadLocal may not be robust enough-- you can't have more than one Object in your threadlocal scope the way it's set up there.  You probably want a Map keyed on Key.

Check out the Scopes page on the wiki:

http://code.google.com/p/google-guice/wiki/Scopes

[Alen Vrečko]

I might not see the point but why do we need to store the providers,
map?

Somebody correct me if I am mistaken in any case will learn something
new.

As I see it, scope is basically just one provider that wraps around
original "no scope" provider.

No_Scope_Provider <-> Scoped_Provider <-> new instance

So when you call bind(...).in(MY_SCOPE), Guice will call the scope
method (just once) on your Scope and use the returned provider instead
of the default (new instance each time).

In your case, I think you essentially return the get(){ return
unscoped.get())} meaning ThreadLocalScope is actually NO_SCOPE.

Haven't really worked with ThreadLocal but how about this:

public <T> Provider<T> scope(Key<T> key, final Provider<T> unscoped)
{

return new Provider<T>() {
private final ThreadLocal<T> threadData = new
ThreadLocal<T>();

public T get() {

T instance = threadData.get();

if (instance == null) {
instance = unscoped.get();
threadData.set(instance);
}

return instance;
}
};

}



Cheers,
Alen

[Rahul]

Thanks Logan, Alen - appreciate the explanations.

Alen's solution is closer to what I was looking for, but I do see the
point Logan made in his suggestion of using a Map keyed with Key. I
can now apply a @ThreadLocalScoped annotation to any Entity... \o/

Cheers,
Rahul

PS: Google forums should provide some sorta code formatting :-)

[Bob Lee]

Are you sure you actually want thread local scope? It's very prone to memory leaks.

Bob

[Josh McDonald]

*nods*

Nearly every time I've seen ThreadLocal used, it's to do something that would be better implemented with a more specific scope - such as getting session or application values into unrelated components (before widespread adoption of DI).

-Josh

On Wed, Oct 1, 2008 at 8:59 AM, Bob Lee <craz...@crazybob.org> wrote:

Are you sure you actually want thread local scope? It's very prone to memory leaks.

Bob

--
"Therefore, send not to know For whom the bell tolls. It tolls for thee."

http://flex.joshmcdonald.info/

:: Josh 'G-Funk' McDonald
:: 0437 221 380 :: jo...@gfunk007.com

[Rahul]

Yes, out of curiosity I did want to implement a thread local scope
with Guice. But cheers for pointing out the possible memory leaks - is
there an alternative to it?

Rahul

[Brian Pontarelli]

Is this still true in an environment that can be configured with fixed
thread pools such as servlet containers and such? I was under the
impression that the Map used was keyed off the thread ID and that was
fixed. Plus I also thought they were all weak references.

I could see some possibility of issues if your thread pools shrink and
grow, but as long as the references are traversable and self contained
not bound to any long lived object, the weak references in the map
should work as expected, correct?

-bp

[Stuart McCulloch]

2008/10/1 Brian Pontarelli <br...@pontarelli.com>

Is this still true in an environment that can be configured with fixed
thread pools such as servlet containers and such? I was under the
impression that the Map used was keyed off the thread ID and that was
fixed. Plus I also thought they were all weak references.

not weak references as such, from the ThreadLocal javadoc:

  "Each thread holds an implicit reference to its copy of a thread-local
  variable as long as the thread is alive and the ThreadLocal instance
  is accessible; after a thread goes away, all of its copies of thread-local
  instances are subject to garbage collection (unless other references
  to these copies exist)."

so it depends on the life of the thread - usually the leaking thread
is the primordial thread that started the application, which is often
long-lived.

the other common leak is when you don't clear the thread-local
before returning a thread to its pool - this could keep alive a lot
of data until the thread is pulled out of the pool and re-assigned
(and the thread local is reset)

I could see some possibility of issues if your thread pools shrink and
grow, but as long as the references are traversable and self contained
not bound to any long lived object, the weak references in the map
should work as expected, correct?

except the references aren't weak, but tied to the life of the thread
 

-bp

On Sep 30, 2008, at 4:59 PM, Bob Lee wrote:

> Are you sure you actually want thread local scope? It's very prone
> to memory leaks.
>
> Bob
>
> >

--
Cheers, Stuart

[Brian Pontarelli]

But the ThreadLocal only stores a single pointer. Therefore, if you set the pointer, forget to clear it, check into the pool and then check out again, once the thread comes out of the pool that pointer is still valid and if you replace it, they old value is available for GC. The leak is only as bad as the number of threads you have and what they are pointing to, IFF you don't release the ThreadLocal value at the end of the conversation with the thread. If you release ThreadLocal values after the end of the conversation with the thread, you should be fine.

Again, correct me if I'm wrong.

-bp

[Dhanji R. Prasanna]

On Thu, Oct 2, 2008 at 12:23 AM, Brian Pontarelli <br...@pontarelli.com> wrote:

I could see some possibility of issues if your thread pools shrink and
grow, but as long as the references are traversable and self contained
not bound to any long lived object, the weak references in the map
should work as expected, correct?

Also if you don't clear the value at the end of a request (for instance), it could potentially hang around and show up in another request when the pool thread is resurrected.

Dhanji.

[Brian Pontarelli]

Right. That's what I was referring to here:

"Therefore, if you set the pointer, forget to clear it, check into the pool and then check out again, once the thread comes out of the pool that pointer is still valid and if you replace it, they old value is available for GC."

That still doesn't mean a leak unless the pointer references an object that continues to grow and is never cleared.

-bp

[Dhanji R. Prasanna]

On Fri, Oct 3, 2008 at 12:11 AM, Brian Pontarelli <br...@pontarelli.com> wrote:

Right. That's what I was referring to here:

"Therefore, if you set the pointer, forget to clear it, check into the pool and then check out again, once the thread comes out of the pool that pointer is still valid and if you replace it, they old value is available for GC."

That still doesn't mean a leak unless the pointer references an object that continues to grow and is never cleared.

Yea but it's a scoping leak which is far worse =D

Dhanji.

[Brian Pontarelli]

> Yea but it's a scoping leak which is far worse =D

Definitely really nasty, but worse.... that's debatable. VMs don't
like OutOfMemoryErrors much. All those other application errors, they
don't mind so much. ;)

-bp

[Bob Lee]

My point is just that "thread local" is typically too low level of an abstraction. Most times, you probably want a higher level abstraction like "request" scope.

The only remaining place I can think of where you might actually want "thread local" scope is for a sort of thread local cache. For example, you might use it for a DateFormat object (which can be reused but only by one thread at a time).

If your thread local scope keeps strong references to the values, it won't leak indefinitely (because you have a cap of N threads * M cached items), but it will raise the base memory footprint of your app higher than it needs to be. I've definitely had thread local "leaks" that ran my application out of memory.

Perhaps you could tell us more about how you plan to use a thread local scope, but if it's just for thread local caching, maybe you could call it ThreadLocalCacheScope and keep soft references to the values.

Bob

[Dhanji R. Prasanna]

To beat a dead horse: leaking user data is a security problem and is *much* worse than running OOM and crashing (which is unlikely given that JVMs have a limit of 15K threads, even if you assume 1 instance leak per TL).

Dhanji.

[Brian Pontarelli]

We can beat the horse. ;) I use ThreadLocals for most conversation scope objects, request, entitymanager, etc. You sorta have to because JEE, well, sort sucks. If you are calling these user data, then yeah, it could happen, but just clean up after yourself. You should probably be fully capable of handling conversation boundaries inside and outside JEE. I wouldn't put much of anything else in there though.

So, I'm not convinced that leaking user data just happens and that is this major issue you are making it out to be. I consider a JVM crash from an OOM error the worse that can happen because it is often fatal.

On most platforms the threads limits are control by memory, files descriptors, the kernel, etc. The VM will crash and not recover for a number of reasons and OOM is one of them. OOM errors can occur for other reasons besides the inability to create a thread, which on some platforms happens *way* below 15K. But, if your app is crashing from thread creation issues, you got serious architecture problems.

All that being said, if you create a TL scope and misuse it, you got issues. If you setup a cache and misuse it, you got issues. Which is worse, I say OOM over TL almost all the time.

-bp

[Brian Pontarelli]

> If your thread local scope keeps strong references to the values, it
> won't leak indefinitely (because you have a cap of N threads * M
> cached items), but it will raise the base memory footprint of your
> app higher than it needs to be. I've definitely had thread local
> "leaks" that ran my application out of memory.

Right. That was my only point really. :)

-bp

[Bob Lee]

I should also note that thread local scope was one of the first scopes I had in Guice. It's so easy to implement, why not support it? But then I realized that it was error prone and that I didn't actually have any good use cases, so I took it out, and I haven't needed it since. Providing features just because we can isn't the Guice way. YMMV.

Bob

[Brian Pontarelli]

Been thinking on this stuff a bit this morning and I think I could
remove my ThreadLocals and put stuff in a Request scope for my
webapps. As long as I could figure out a way to lazy load from the
scope since I have some cases where I inject something that requires
an EntityManager (for example) before the EntityManager is setup.

-bp

[Bob Lee]

On Fri, Oct 3, 2008 at 8:02 AM, Brian Pontarelli <br...@pontarelli.com> wrote:

Been thinking on this stuff a bit this morning and I think I could
remove my ThreadLocals and put stuff in a Request scope for my
webapps. As long as I could figure out a way to lazy load from the
scope since I have some cases where I inject something that requires
an EntityManager (for example) before the EntityManager is setup.

You could inject Provider<EntityManager>.

Bob

[Brian Pontarelli]

Yeah, went that route and for a number of somewhat lame reasons, that ended up failing and I had to resort to the proxy solution. I ended up having to proxy EntityManager and HttpServletRequest (sorta). Those are really the only two tricky problems I had with JCatapult.

-bp