clj-commons/claypoole

Upgrade to clojure 1.9 or 1.10 and jdk 8

ieugen opened this issue · 10 comments

I believe ewe should upgrade since I can't even find artifacts (circle-ci docker build images) for clojure 1.8.
The same for jdk 8.
Right now claypool seems to specify java 6 but there are classes imported that belong to JDK 7 and JDK 8: LinkedBlockingQueue, CompletableFuture .

Upgraded to jdk8 build and clojure 1.9 238e273 .

Clojure 1.10.x tests fail:

FAIL in (test-pvalues-buffer) (claypoole_test.clj:495)
pvalues-buffer handles input exceptions correctly
expected: (thrown-with-msg? Exception #"deliberate" (dorun (pmap-like pool inc inputs)))
  actual: #error {
 :cause "deliberate exception"
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "Syntax error compiling at (/tmp/form-init5034380779812879855.clj:1:6367)."
   :data #:clojure.error{:phase :compile-syntax-check, :line 1, :column 6367, :source "/tmp/form-init5034380779812879855.clj"}
   :at [clojure.lang.Compiler analyze "Compiler.java" 6808]}
  {:type java.lang.Exception
   :message "deliberate exception"
   :at [com.climate.claypoole_test$check_input_exception$fn__1220 invoke "claypoole_test.clj" 490]}]
 :trace
 [[com.climate.claypoole_test$check_input_exception$fn__1220 invoke "claypoole_test.clj" 490]
  [clojure.core$map$fn__5851 invoke "core.clj" 2753]
  [clojure.lang.LazySeq sval "LazySeq.java" 42]
  [clojure.lang.LazySeq seq "LazySeq.java" 51]
  [clojure.lang.RT seq "RT.java" 531]
  [clojure.core$seq__5387 invokeStatic "core.clj" 137]
  [clojure.core$seq__5387 invoke "core.clj" 137]
  [com.climate.claypoole.lazy_test$fn__2389$pmap_like__2390$iter__2391__2395$fn__2396 invoke "lazy_test.clj" 171]
  [clojure.lang.LazySeq sval "LazySeq.java" 42]
  [clojure.lang.LazySeq seq "LazySeq.java" 51]
  [clojure.lang.RT seq "RT.java" 531]
  [clojure.core$seq__5387 invokeStatic "core.clj" 137]
  [clojure.core$concat$cat__5480$fn__5481 invoke "core.clj" 734]
  [clojure.lang.LazySeq sval "LazySeq.java" 42]
  [clojure.lang.LazySeq seq "LazySeq.java" 51]
  [clojure.lang.ChunkedCons chunkedNext "ChunkedCons.java" 59]
  [clojure.lang.ChunkedCons next "ChunkedCons.java" 43]
  [clojure.lang.RT countFrom "RT.java" 649]
  [clojure.lang.RT count "RT.java" 639]
  [clojure.lang.Cons count "Cons.java" 49]
  [clojure.lang.Compiler analyze "Compiler.java" 6780]
  [clojure.lang.Compiler analyze "Compiler.java" 6745]
  [clojure.lang.Compiler$BodyExpr$Parser parse "Compiler.java" 6120]
  [clojure.lang.Compiler$FnMethod parse "Compiler.java" 5467]
  [clojure.lang.Compiler$FnExpr parse "Compiler.java" 4029]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 7104]
  [clojure.lang.Compiler analyze "Compiler.java" 6789]
  [clojure.lang.Compiler analyze "Compiler.java" 6745]
  [clojure.lang.Compiler$BodyExpr$Parser parse "Compiler.java" 6120]
  [clojure.lang.Compiler$FnMethod parse "Compiler.java" 5467]
  [clojure.lang.Compiler$FnExpr parse "Compiler.java" 4029]
  [clojure.lang.Compiler analyzeSeq "Compiler.java" 7104]
  [clojure.lang.Compiler analyze "Compiler.java" 6789]
  [clojure.lang.Compiler eval "Compiler.java" 7173]
  [clojure.lang.Compiler eval "Compiler.java" 7131]
  [clojure.core$eval invokeStatic "core.clj" 3214]
  [clojure.core$eval invoke "core.clj" 3210]
  [com.climate.claypoole.lazy_test$fn__2389$pmap_like__2390 invoke "lazy_test.clj" 167]
  [com.climate.claypoole.lazy_test$check_all_buffer$fn__2222 invoke "lazy_test.clj" 87]
  [com.climate.claypoole_test$check_input_exception$fn__1222 invoke "claypoole_test.clj" 497]
  [com.climate.claypoole_test$check_input_exception invokeStatic "claypoole_test.clj" 495]
  [com.climate.claypoole_test$check_input_exception invoke "claypoole_test.clj" 484]
  [com.climate.claypoole_test$check_all$fn__1393 invoke "claypoole_test.clj" 702]
  [com.climate.claypoole_test$check_all invokeStatic "claypoole_test.clj" 701]
  [com.climate.claypoole_test$check_all invoke "claypoole_test.clj" 682]
  [com.climate.claypoole.lazy_test$check_all invokeStatic "lazy_test.clj" 51]
  [com.climate.claypoole.lazy_test$check_all invoke "lazy_test.clj" 48]
  [com.climate.claypoole.lazy_test$check_all_buffer invokeStatic "lazy_test.clj" 87]
  [com.climate.claypoole.lazy_test$check_all_buffer invoke "lazy_test.clj" 85]
  [com.climate.claypoole.lazy_test$fn__2389 invokeStatic "lazy_test.clj" 174]
  [com.climate.claypoole.lazy_test$fn__2389 invoke "lazy_test.clj" 164]
  [clojure.test$test_var$fn__9707 invoke "test.clj" 717]
  [clojure.test$test_var invokeStatic "test.clj" 717]
  [clojure.test$test_var invoke "test.clj" 708]
  [clojure.test$test_vars$fn__9733$fn__9738 invoke "test.clj" 735]
  [clojure.test$default_fixture invokeStatic "test.clj" 687]
  [clojure.test$default_fixture invoke "test.clj" 683]
  [clojure.test$test_vars$fn__9733 invoke "test.clj" 735]
  [clojure.test$default_fixture invokeStatic "test.clj" 687]
  [clojure.test$default_fixture invoke "test.clj" 683]
  [clojure.test$test_vars invokeStatic "test.clj" 731]
  [clojure.test$test_all_vars invokeStatic "test.clj" 737]
  [clojure.test$test_ns invokeStatic "test.clj" 758]
  [clojure.test$test_ns invoke "test.clj" 743]

So thanks to community help I managed to figure out we are hitting this https://clojure.atlassian.net/browse/CLJ-2499 .
eval behaviour changed and now we get an exception wrapped in a compiler exception.

Not sure how to fix it.
Clojurians discussion https://clojurians.slack.com/archives/C03S1KBA2/p1644501275732479 .

https://github.com/clojure/clojure/blob/master/test/clojure/test_helper.clj#L134-L151

https://clojurians-log.clojureverse.org/clojure/2022-02-10

Hey @leon-barrett , if you have any ideas on how to solve upgrade to 1.10 , please share.
I don't know how to use macros so I am a bit lost.
I guess I have to make the leap at one point.

So, I think this is a real test failure. I think we might need to unwrap the compiler exception and re-raise the actual exception.

Why do I want that? I want using thread pools to be pretty transparent--if you raise an exception in your pmap, I want you to be able to catch that exception. (That's why deref-fixing-exceptions exists https://github.com/clj-commons/claypoole/blob/master/src/clj/com/climate/claypoole/impl.clj#L51 .) We might need to update that to unwrap the CompilerException? I'll have to take a look tonight.

Thanks for the insight, it's very valuable.
If you don't have time I will take a crack at it in the next week or so.
Please let me know what you decide.

I don't think deref-fixing-exceptions will work in this case since the exception is thrown during macro expansion.
No code is executed at that time.

Yeah, I looked at it some, and I think disabling the tests for just those cases where we eval is probably the right thing. I made a simple PR for it: #62 , but there might be a tidier way to do it.

Using deps aliases (lein profiles) we can support multiple clojure versions for tests and development.

This has been fixed in code, we should close this issue when we include the different clojure versions in automated ci builds.

Was resolved via CI tests with multiple clojure versions (lein profiles)