ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Павлухин Иван <vololo...@gmail.com>
Subject Re: Class field ThreadLocal. Why not static?
Date Tue, 11 Sep 2018 10:02:15 GMT
Alex,

ThreadLocal subclass is used in IgniteH2Indexing for simple access to H2
Connection from current thread. Such subclass has a capability to create
connection if one does not exist, so obtaining connection is merely
ThreadLocal.get. Also there are scheduled routines to cleanup connections
and associated with them statement cache after some expiration time. For
that reason Map<Thread, H2ConnectionWrapper> is maintained. As query can
run on user thread we need to cleanup mentioned map to avoid a leak when
Thread is terminated. So we need to check thread status in cleanup routines
and remove entries for terminated Threads. And historically there was no
cleanup for terminated threads and leak was possible. And also great care
must be taken in order to avoid cyclic reference between ThreadLocal
instance and a stored value. Which easily could occur if the stored value
is covered by multiple layers of abstraction.

And I am describing some historical state. Now machinery in IgniteH2Indexing
is even more complex (I hope we will have a chance to improve it).

2018-09-11 11:00 GMT+03:00 Alexey Goncharuk <alexey.goncharuk@gmail.com>:

> Ivan,
>
> Can you elaborate on the issue with the thread local cleanup you've faced?
>
> вт, 11 сент. 2018 г. в 9:13, Павлухин Иван <vololo100@gmail.com>:
>
> > Guys,
> >
> > As we know ThreadLocal is an instrument which should be used with great
> > care. And I recently faced with problems related to proper cleanup of
> > ThreadLocal which is not needed anymore. In my opinion the best thing (in
> > ideal world) is to get rid of ThreadLocal where possible, but I guess
> that
> > it is quite hard (in real world).
> >
> > Also, a question comes to my mind. As ThreadLocal is so common in our
> code,
> > could you suggest some guidance or code fragments which address proper
> > ThreadLocal
> >  lifecycle control and especially cleanup?
> >
> > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk <alexey.goncharuk@gmail.com
> >:
> >
> > > Maxim,
> > >
> > > Ignite supports starting multiple instances of Ignite in the same VM,
> so
> > > having static thread locals for the fields you mentioned does not work.
> > >
> > > Generally, I think thread-local should be bound to the lifespan of the
> > > component it describes. Static thread-locals are hard to clean-up and
> > they
> > > often lead to leaks, so I would rather changed existing static
> > > thread-locals to be non-static.
> > >
> > > --AG
> > >
> > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov <maxmuzaf@gmail.com>:
> > >
> > > > Igniters,
> > > >
> > > > According to javadoc [1] class ThreadLocal:
> > > > `ThreadLocal instances are typically private *static* fields in
> classes
> > > > that wish to associate state with a thread (e.g., a user ID or
> > > Transaction
> > > > ID).`
> > > >
> > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per
> class
> > > > instance`. What the real cases of using non-static ThreadLocal class
> > > fields
> > > > in Ignite code project? When we need it?
> > > >
> > > > In Ignite code project I've found ThreadLocal usage as:
> > > >  - non-static - 67
> > > >  - static  - 68
> > > >
> > > > Back to my example, I've checked FileWriteAheadLogManager. It has:
> > > > 1) private final ThreadLocal<Boolean> interrupted [2]
> > > > 2) private final ThreadLocal<WALPointer> lastWALPtr [3]
> > > > I think both of these fields should be set and used as `static`. Can
> > > anyone
> > > > confirm it?
> > > >
> > > >
> > > > [1]
> > https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html
> > > > [2]
> > > >
> > > > https://github.com/apache/ignite/blob/master/modules/
> > > core/src/main/java/org/apache/ignite/internal/processors/
> > > cache/persistence/wal/FileWriteAheadLogManager.java#L253
> > > > [3]
> > > >
> > > > https://github.com/apache/ignite/blob/master/modules/
> > > core/src/main/java/org/apache/ignite/internal/processors/
> > > cache/persistence/wal/FileWriteAheadLogManager.java#L340
> > > > --
> > > > --
> > > > Maxim Muzafarov
> > > >
> > >
> >
> >
> >
> > --
> > Best regards,
> > Ivan Pavlukhin
> >
>



-- 
Best regards,
Ivan Pavlukhin

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message