Skip to content

Conversation

@jake-jake-jake
Copy link
Contributor

I can add some more test cases for these if it makes sense to do so; these are the more obvious ones that occurred to me.

@jeaye
Copy link
Member

jeaye commented Jan 10, 2025

Great tests. What do you think about adding some tests for binding conveyance across threads? When a thread is created, it should copy the current binding state and changes made to bindings should only be visible on the current thread.

@jake-jake-jake
Copy link
Contributor Author

Great tests. What do you think about adding some tests for binding conveyance across threads? When a thread is created, it should copy the current binding state and changes made to bindings should only be visible on the current thread.

Yeap, happy to add those. I wasn't sure we wanted to mess with threads in these, but if we are, we should test those cases.

I put up some that test threaded bindings via futures as well as how delay handles them. The tests are failing in CI, and I'll dig into a possible solution later this weekend/early next week, unless we already have a workaround for threaded execution. A quick search turns this up, which I think might be the cause (since the tests pass in my CIDER session).

@jake-jake-jake
Copy link
Contributor Author

I pushed up a hack to make this pass. It's not the most satisfying solution, but it does make things work for now. If there is a better way to direct lein to run specific tests alone, I am happy to change this to do that but I didn't see anything obvious in the docs or through digging around.

@jeaye
Copy link
Member

jeaye commented Jan 14, 2025

Nice! We just merged this PR: #37

Would you mind wrapping your new binding file in the same macro before I merge?

@jake-jake-jake
Copy link
Contributor Author

Done!

(let [derefed-f @f]
(t/is (= :callee @derefed-f) "Binding in futures preserved.")))))

(shutdown-agents)))
Copy link
Member

@jeaye jeaye Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this will cause issues with any other tests using threads. Does it make sense to add a shutdown hook instead?

; In portability.cljc
(defn shutdown-agents-on-close []
  #?(:clj (.addShutdownHook (Runtime/getRuntime) (Thread. ^Runnable shutdown-agents))))

Taken from here: https://medium.com/helpshift-engineering/achieving-graceful-restarts-of-clojure-services-b3a3b9c1d60d

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like that was what was happening with the threads in these tests! There was a different shutdown agents call in add_watch.cljc. I removed it and these tests pass without any issues now.

@jeaye jeaye merged commit 4b7e161 into jank-lang:main Jan 15, 2025
2 checks passed
@jeaye
Copy link
Member

jeaye commented Jan 15, 2025

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants