puniverse / pulsar

Fibers, Channels and Actors for Clojure
http://docs.paralleluniverse.co/pulsar/
Other
910 stars 53 forks source link

Problems with defsfn #45

Closed hsgrott closed 8 years ago

hsgrott commented 8 years ago

I was running in the REPL some examples derived from this post:

http://yogthos.net/posts/2015-06-17-Using-Pulsar.html

Problem is, every time I redefined one of the suspendable functions (the ones created with defsfn), the -main method would hang, no matter which method was used (clojure.tools.namespace.repl/refresh, using CIDER commands, etc).

I've looked into the definition of the defsfn macro, and found the following:

(defmacro defsfn
  "Defines a suspendable function that can be used by a fiber or actor.
  Used exactly like `defn`"
  [& expr]
  `(do
     (defn ~@expr)
     (def ~(first expr) (suspendable! ~(first expr)))))

It seems that there is some kind of weird bug/interaction going on (or maybe I'm extremely unlucky :D). Anyways, here is an alternative version that seems to correct the problem:

(defmacro defsfnp
  "Walks through a 'defn' macroexpansion and wraps the generated fn* in
  a 'suspendable!' call." 
  [& expr]
  (-> (cons 'defn expr)
      walk/macroexpand-all
      zip/seq-zip
      zip/down
      zip/rightmost
      (zip/edit (fn [loc] `(suspendable! ~loc)))
      zip/root))

It might be a good idea to look up the actual fn* node instead of using zip/rightmost to make this macro more robust, but that is left as an exercise to the reader.

The code was tested in an Ubuntu 15.10 machine (Linux 4.2.0-18-generic #22-Ubuntu SMPx86_64 x86_64 x86_64 GNU/Linux) and a Mac using OSX El Capitan, both running JDK 1.8.0_66-b17, Clojure 1.7.0, pulsar 0.7.3 and quasar-core 0.7.3, with the same results.

Here is a gist with a sample project.clj, a sample test file and a bunch of tests adapted from the "def" tests in the main clojure repo:

https://gist.github.com/anonymous/dc9f501e8c52b3a7166c

I hope it helps.

circlespainter commented 8 years ago

Thanks, even in a simple lein repl the "bugged" main seems to hang. But it looks like simply avoiding the re-definition of the var in the original defsfn macro solves the problem (and the Pulsar testsuite runs ok), at least with your test project. Can you confirm it works for you?

hsgrott commented 8 years ago

Hello @circlespainter,

Thank you for your answer.

I didn't understand your advice. When you say to avoid the re-definition of the var in the original defsfn macro, you mean removing the def part? I've tried that, but it didn't work properly. If the macro is defined like:

(defmacro defsfn
  "Defines a suspendable function that can be used by a fiber or actor.
  Used exactly like `defn`"
  [& expr]
  `(do
     (defn ~@expr))))

It would be a normal defn. But if the functions are defined with that macro (or directly with defn), similar problems arise. I've noticed that, in this particular case, spawn marks the function as suspendable when called with only one argument (like (spawn buggy-pong)), but will not do that when called with more than one argument (as in (spawn buggy-ping 3 a1)); you can test that by changing those functions to use defn and following it by calling suspendable? on them right after starting a fresh REPL. If you call (suspendable! buggy-ping) directly in the REPL after a fresh run, buggy-main will work as intended (or will not be buggy anymore :D).

The way things are right now, defsfn will work when you don't have a var redefinition (when you don't reload your code). It might pose no problems when deploying a new application or if you develop without using a REPL, but it also means that you can't use a REPL to experiment with Pulsar (at least in this specific configuration). I don't know if this is the intended behavior, a deliberate decision based on the design of Quasar/Pulsar (considering the instrumentation stuff), a bug (in spawn, in defsfn or both), or me doing things improperly.

I'm new to Pulsar, so I apologize if I'm getting something wrong or mixing things up.

Best regards.

circlespainter commented 8 years ago

Sorry, I didn't explain thoroughly. I mean this:

(defmacro defsfn
  "Defines a suspendable function that can be used by a fiber or actor.
  Used exactly like `defn`"
  [& expr]
  `(do
     (defn ~@expr)
     (suspendable! ~(first expr))))

This is enough for me to make it work in the REPL and to fix your example while still having the testsuite fully ok.

hsgrott commented 8 years ago

Hi @circlespainter,

It's working now. Thank you very much.

There's only the situation with the spawn function. It seems that it doesn't mark a regular function as suspendable when called with more than one argument.

Best regards.

circlespainter commented 8 years ago

My pleasure! Would you mind opening a separate issue for spawn?

hsgrott commented 8 years ago

Done.

circlespainter commented 8 years ago

Thanks!