ClojureScript

cljs.closure/build does not find upstream dependencies when called from worker thread

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None

Description

Example stacktrace: https://www.refheap.com/97198 (context is using figwheel in a clojure REPL from CIDER)

Because cljs.closure/build calls the 0-arity form of get-upstream-deps, it is implicitly using the current thread's classloader to find the deps.cljs resources. It is then assoc'ing the result into opts, so the caller has no way of gathering the dependencies themselves and passing them in.

Activity

Hide
Michael Griffiths added a comment -

Patch CLJS-1036--001.patch, attached, uses merge instead of assoc to allow passing of the upstream dependency data via opts.

Show
Michael Griffiths added a comment - Patch CLJS-1036--001.patch, attached, uses merge instead of assoc to allow passing of the upstream dependency data via opts.
Hide
Michael Griffiths added a comment -

We could also allow passing of a :resources-classloader opt so callers don't have to gather the dependencies themselves, if you'd prefer a patch that does that.

Show
Michael Griffiths added a comment - We could also allow passing of a :resources-classloader opt so callers don't have to gather the dependencies themselves, if you'd prefer a patch that does that.
Hide
Bruce Hauman added a comment -

This is a problem with calling cljs.closure/build inside nREPL in general when upstream deps are involved.

Show
Bruce Hauman added a comment - This is a problem with calling cljs.closure/build inside nREPL in general when upstream deps are involved.
Hide
David Nolen added a comment -

I would like Chas Emerick to chime in on this thread before considering it.

Show
David Nolen added a comment - I would like Chas Emerick to chime in on this thread before considering it.
Hide
Chas Emerick added a comment -

Could someone point me to a simple (sample?) project that uses upstream dependencies? I haven't used them yet, and don't have such a thing handy.

The proposed patch is confusing to me; if the problem is in resolving dependencies due to classloader state/configuration, how does it help to give opts the opportunity to clobber what get-upstream-deps returns? That is, doesn't this just move the problem downstream to who/whatever is calling build?

Show
Chas Emerick added a comment - Could someone point me to a simple (sample?) project that uses upstream dependencies? I haven't used them yet, and don't have such a thing handy. The proposed patch is confusing to me; if the problem is in resolving dependencies due to classloader state/configuration, how does it help to give opts the opportunity to clobber what get-upstream-deps returns? That is, doesn't this just move the problem downstream to who/whatever is calling build?
Hide
Michael Griffiths added a comment -

Chas: yes, the proposed patch simply moves the responsibility to the caller. I was under the assumption that changing how get-upstream-deps/build resolves the classloader by default would be too-breaking of a change, and had not even considered the fact that this might be a broader issue with nREPL itself.

Example usage of get-upstream-deps in the wild (that even mentions classloader issues): https://github.com/immutant/immutant-release/blob/ea538feb548bde86ebce20ec679da7a19b639259/integration-tests/apps/ring/basic-ring/src/basic_ring/core.clj#L51

Note that Weasel (at least) is currently gathering the upstream deps itself too: https://github.com/tomjakubowski/weasel/commit/5cd7009f584100f0d89fc7f00079458bb1f2c016

I'll set up an example project tonight if you like, but I think all you need is to add [cljsjs/react "0.12.2-5"] as a dependency to an existing cljs project and then require [cljsjs.react] in one of its namespaces.

Show
Michael Griffiths added a comment - Chas: yes, the proposed patch simply moves the responsibility to the caller. I was under the assumption that changing how get-upstream-deps/build resolves the classloader by default would be too-breaking of a change, and had not even considered the fact that this might be a broader issue with nREPL itself. Example usage of get-upstream-deps in the wild (that even mentions classloader issues): https://github.com/immutant/immutant-release/blob/ea538feb548bde86ebce20ec679da7a19b639259/integration-tests/apps/ring/basic-ring/src/basic_ring/core.clj#L51 Note that Weasel (at least) is currently gathering the upstream deps itself too: https://github.com/tomjakubowski/weasel/commit/5cd7009f584100f0d89fc7f00079458bb1f2c016 I'll set up an example project tonight if you like, but I think all you need is to add [cljsjs/react "0.12.2-5"] as a dependency to an existing cljs project and then require [cljsjs.react] in one of its namespaces.
Hide
Bruce Hauman added a comment - - edited

Chas, David here is a very quick example:

If you do a

lein new om-mies hello-world
cd hello-world
lein repl
user> (require '[cljs.closure :as cc])
user> (cc/build "src" {})

Assuming lein repl started an nREPL repl. This will produce the error mentioned above. The problem is that om is utilizing the new deps.js facility to autmatically import cljsjs.react. get-upstream-deps depends on being run on the main thread.

https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/closure.clj#L1116

A solution that fixes this in get-upstream-deps* would be preferable.

Is this simple solution to use the system classloader when not on the main thread:

(if (not= (.getName (Thread/currentThread)) "main")
  (java.lang.ClassLoader/getSystemClassLoader)
  (. (Thread/currentThread) (getContextClassLoader)))

To naive? My experience with Java threads and classloaders is minimal.

Or should we always use the (java.lang.ClassLoader/getSystemClassLoader) in this case?

Show
Bruce Hauman added a comment - - edited Chas, David here is a very quick example: If you do a
lein new om-mies hello-world
cd hello-world
lein repl
user> (require '[cljs.closure :as cc])
user> (cc/build "src" {})
Assuming lein repl started an nREPL repl. This will produce the error mentioned above. The problem is that om is utilizing the new deps.js facility to autmatically import cljsjs.react. get-upstream-deps depends on being run on the main thread. https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/closure.clj#L1116 A solution that fixes this in get-upstream-deps* would be preferable. Is this simple solution to use the system classloader when not on the main thread:
(if (not= (.getName (Thread/currentThread)) "main")
  (java.lang.ClassLoader/getSystemClassLoader)
  (. (Thread/currentThread) (getContextClassLoader)))
To naive? My experience with Java threads and classloaders is minimal. Or should we always use the (java.lang.ClassLoader/getSystemClassLoader) in this case?
Hide
Chas Emerick added a comment -

Using the system classloader isn't necessary. get-upstream-deps is currently using .findResources, not .getResources; the difference is that the former looks only for matching resources in the target classloader, while the latter first delegates to the parent classloader, and only looks locally if nothing is found there. So, just using .getResources instead will make all current usage work as expected, I think. e.g.

;; `lein repl` nREPL here
user=> (enumeration-seq (.getResources (. (Thread/currentThread) (getContextClassLoader))  "deps.cljs"))
(#<URL jar:file:/home/chas/.m2/repository/cljsjs/react/0.12.2-5/react-0.12.2-5.jar!/deps.cljs>)
user=> (enumeration-seq (.findResources (. (Thread/currentThread) (getContextClassLoader))  "deps.cljs"))
nil

There are some classloader edge cases that might motivate making get-upstream-deps perform a more comprehensive search, explicitly pinging all classloaders for matching resources. This is sometimes necessary for these sorts of scanning scenarios if e.g. a deps.cljs file is available via a parent classloader, which will therefore "shadow" other deps.cljs files in child classloaders. In that case, you need something like cemerick.pomegranate/resources.

Hope that helps.

Show
Chas Emerick added a comment - Using the system classloader isn't necessary. get-upstream-deps is currently using .findResources, not .getResources; the difference is that the former looks only for matching resources in the target classloader, while the latter first delegates to the parent classloader, and only looks locally if nothing is found there. So, just using .getResources instead will make all current usage work as expected, I think. e.g.
;; `lein repl` nREPL here
user=> (enumeration-seq (.getResources (. (Thread/currentThread) (getContextClassLoader))  "deps.cljs"))
(#<URL jar:file:/home/chas/.m2/repository/cljsjs/react/0.12.2-5/react-0.12.2-5.jar!/deps.cljs>)
user=> (enumeration-seq (.findResources (. (Thread/currentThread) (getContextClassLoader))  "deps.cljs"))
nil
There are some classloader edge cases that might motivate making get-upstream-deps perform a more comprehensive search, explicitly pinging all classloaders for matching resources. This is sometimes necessary for these sorts of scanning scenarios if e.g. a deps.cljs file is available via a parent classloader, which will therefore "shadow" other deps.cljs files in child classloaders. In that case, you need something like cemerick.pomegranate/resources. Hope that helps.
Hide
David Nolen added a comment -

Chas's suggested fix has been committed to master. It works on the simple tests I've run but I'd like to hear more confirmation. Thanks everyone.

https://github.com/clojure/clojurescript/commit/79208f5bf15825a2ba2d0ce95aae6d2b71966494

Show
David Nolen added a comment - Chas's suggested fix has been committed to master. It works on the simple tests I've run but I'd like to hear more confirmation. Thanks everyone. https://github.com/clojure/clojurescript/commit/79208f5bf15825a2ba2d0ce95aae6d2b71966494
Hide
David Nolen added a comment -

Closing this one. Chas's solution is the correct one.

Show
David Nolen added a comment - Closing this one. Chas's solution is the correct one.

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: