Clojurians Log v2

Thank you so much @borkdude! I will test this in the next week and update my repos! 😄

@h0bbit you mentioned this on reClojure, so here you go!

That should allow promesa to inherit future fixes to binding-conveyor-fn. IIRC it has a memory leak.

@ambrosebs wasn't this fixed in: https://github.com/clojure/clojure/commit/12f07da889819bc5613546ec223e97ac27c86dbf

Different one, but I forgot the fix didn't belong in binding-conveyor-fn. https://clojure.atlassian.net/browse/CLJ-2619

The caller needs to clear the bindings manually. Maybe promesa also has this leak.

the memory leak happens because the thread used is pooled so it still active and its thread local still points to the captured frame

interesting

i guess when the thread is stoped for inactivity or the frame is overwritten with other value, then, the previous object will be available for GC

This problem (memory leak) should not happen when used with VirtualThreads because they are not pooled by its nature

VT all the things!

True, an efficient fix would only clear the binding frame when the thread is pooled.

On the other hand, I don't think the clearing will penalize a lot. I mean the performance overhead is on push/pop operations, if we simply reset it to an empty frame i guess it will not add too much overhead. I will measure it

In CLJ-2619-future-memory-leak-4.patch I wasn't confident enough that it was ok to clear the frame, instead I did this:

(let [o (clojure.lang.Var/getThreadBindingFrame)]
  (try (f)
       (finally (clojure.lang.Var/resetThreadBindingFrame o)))

interesting

I wonder if that reintroduces the previous memory leak.

do you have info/link for the previous memory leak?

https://github.com/clojure/clojure/commit/12f07da889819bc5613546ec223e97ac27c86dbf

Looks like it was related to agents rather than futures.

but I didn't understand how it worked.

yeah, but i think this change will not introduce back the prev memory leak issue

because we just restore on the target thread thre frame previous to reset

the prev memory leak is related to use the same frame from parent thread on the target thread, and this is replaced by clone operation for avoid it

so just a clojure.lang.Var/resetThreadBindingFrame empty-frame should do it?

I think yes

but for make it available on promesa we sill need to reintroduce a local copy of conveyor helper hehe

I think it could be in binding-conveyor-inner fwiw

no problem, leave binding-conveyor-fn* as it is for bb and make a custom one for clj

in the next version of bb clojure.lang.Var/{clone,reset,get}ThreadBindingFrame will work, I'm working on it

oh, I missed that.

I guess with-bindings /`get-bindings` might also work for bb?

@niwinz here's how you do it using reader conditionals:

(def binding-conveyor-fn* #?(:bb @#'clojure.core/binding-conveyor-fn :clj your-custom-version)

yeah sorry I'm muddled up.

I will read this thread later, tomorrow probably (traveling) o/

have fun!

my idea was basically (#?(:bb bound-fn* :default binding-conveyor-fn*) ifn) if you need to experiment before bb supports custom conveyor.

I'd prefer bb then to use just the normal clojure.core/binding-conveyor-fn

I asked claude to write me a test for testing the interop of clojure.lang.Var/.. in bb but it came up with utter nonsense. I wrote this one myself:

(deftest clojure-lang-Var-binding-frame-test
  (is (= [43 42 43 42] (bb nil "(def ^:dynamic *test-var* 42)
   (def results (atom []))
   (binding [*test-var* *test-var*]
    (let [frame (clojure.lang.Var/cloneThreadBindingFrame)]
      (binding [*test-var* 43]
        (let [inner-frame (clojure.lang.Var/getThreadBindingFrame)]
          (swap! results conj *test-var*)
          (clojure.lang.Var/resetThreadBindingFrame frame)
          (swap! results conj *test-var*)
          (clojure.lang.Var/resetThreadBindingFrame inner-frame)
          (swap! results conj *test-var*)))
      (swap! results conj *test-var*)))
   @results"))))

curious how you test cloneThreadBindingFrame. I still don't understand it.

oh that does test it. Rather, test the difference between get/clone.

doh. reading closer. what's the subtleties here? please help me understand frame vs inner-frame?

ah I could add an identical? check for clone

inner-frame has the additional binding

why did you use get vs clone for inner-frame?

(deftest clojure-lang-Var-binding-frame-test
  (is (= [43 42 43 42] (bb nil "(def ^:dynamic *test-var* 42)
   (def results (atom []))
   (binding [*test-var* *test-var*]
    (let [current-frame (clojure.lang.Var/cloneThreadBindingFrame)
          frame (clojure.lang.Var/cloneThreadBindingFrame)]
      (assert (not (identical? current-frame frame)))
      (binding [*test-var* 43]
        (let [inner-frame (clojure.lang.Var/getThreadBindingFrame)]
          (swap! results conj *test-var*)
          (clojure.lang.Var/resetThreadBindingFrame frame)
          (swap! results conj *test-var*)
          (clojure.lang.Var/resetThreadBindingFrame inner-frame)
          (swap! results conj *test-var*)))
      (swap! results conj *test-var*)))
   @results"))))

I used get so I also exercise that method

I'm thinking along the lines of unit testing https://github.com/clojure/clojure/commit/12f07da889819bc5613546ec223e97ac27c86dbf

a unit test that would only pass if it used clone instead of get.

would be sweet if you could come up with one.

agreed!