Browse files

Clear ThreadLocals when no longer needed [CLJ-1125]

Clears transaction and dynamic binding thread locals to prevent memory
leaks when used inside a container. Uses a sentinel frame to prevent
the binding conveyance issues described in CLJ-1299.

Signed-off-by: Stuart Halloway <stu@cognitect.com>
  • Loading branch information...
1 parent ea283da commit 6be05801ff060122bec43e4eac5fdddf5e174fcf @tobias tobias committed with Stuart Halloway Nov 22, 2013
Showing with 39 additions and 26 deletions.
  1. +15 −5 src/jvm/clojure/lang/LockingTransaction.java
  2. +13 −21 src/jvm/clojure/lang/Var.java
  3. +11 −0 test/clojure/test_clojure/parallel.clj
View
20 src/jvm/clojure/lang/LockingTransaction.java
@@ -222,13 +222,23 @@ static LockingTransaction getRunning(){
static public Object runInTransaction(Callable fn) throws Exception{
LockingTransaction t = transaction.get();
- if(t == null)
+ Object ret;
+ if(t == null) {
transaction.set(t = new LockingTransaction());
+ try {
+ ret = t.run(fn);
+ } finally {
+ transaction.remove();
+ }
+ } else {
+ if(t.info != null) {
+ ret = fn.call();
+ } else {
+ ret = t.run(fn);
+ }
+ }
- if(t.info != null)
- return fn.call();
-
- return t.run(fn);
+ return ret;
}
static class Notify{
View
34 src/jvm/clojure/lang/Var.java
@@ -45,35 +45,29 @@ public Object throwArity(int n){
}
static class Frame{
+ final static Frame TOP = new Frame(PersistentHashMap.EMPTY, null);
//Var->TBox
Associative bindings;
//Var->val
// Associative frameBindings;
Frame prev;
-
- public Frame(){
- this(PersistentHashMap.EMPTY, null);
- }
-
public Frame(Associative bindings, Frame prev){
// this.frameBindings = frameBindings;
this.bindings = bindings;
this.prev = prev;
}
protected Object clone() {
- Frame f = new Frame();
- f.bindings = this.bindings;
- return f;
+ return new Frame(this.bindings, null);
}
}
static final ThreadLocal<Frame> dvals = new ThreadLocal<Frame>(){
protected Frame initialValue(){
- return new Frame();
+ return Frame.TOP;
}
};
@@ -96,17 +90,11 @@ protected Frame initialValue(){
//IPersistentMap _meta;
public static Object getThreadBindingFrame(){
- Frame f = dvals.get();
- if(f != null)
- return f;
- return new Frame();
+ return dvals.get();
}
public static Object cloneThreadBindingFrame(){
- Frame f = dvals.get();
- if(f != null)
- return f.clone();
- return new Frame();
+ return dvals.get().clone();
}
public static void resetThreadBindingFrame(Object frame){
@@ -338,10 +326,14 @@ public static void pushThreadBindings(Associative bindings){
}
public static void popThreadBindings(){
- Frame f = dvals.get();
- if(f.prev == null)
- throw new IllegalStateException("Pop without matching push");
- dvals.set(f.prev);
+ Frame f = dvals.get().prev;
+ if (f == null) {
+ throw new IllegalStateException("Pop without matching push");
+ } else if (f == Frame.TOP) {
+ dvals.remove();
+ } else {
+ dvals.set(f);
+ }
}
public static Associative getThreadBindings(){
View
11 test/clojure/test_clojure/parallel.clj
@@ -27,3 +27,14 @@
;; regression fixed in r1218; was OutOfMemoryError
(is (= '(1) (pmap inc [0]))))
+
+(def ^:dynamic *test-value* 1)
+
+(deftest future-fn-properly-retains-conveyed-bindings
+ (let [a (atom [])]
+ (binding [*test-value* 2]
+ @(future (dotimes [_ 3]
+ ;; we need some binding to trigger binding pop
+ (binding [*print-dup* false]
+ (swap! a conj *test-value*))))
+ (is (= [2 2 2] @a)))))

0 comments on commit 6be0580

Please sign in to comment.