<< Back to previous view

[ASYNC-131] go! or "go-now" for CLJS Created: 29/Jun/15  Updated: 06/Jul/15

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

I'd like to see a variant of the "go" macro that executes immediately until the first parkable operation instead of dispatching (effectively calling "setTimeout" twice). This is primarily for CLJS since CLJ can get away with blocking operations but it still might be useful there as well.

My Use-Case:

I currently use core.async to orchestrate CSS animations together and while it works perfectly fine with "go" it does require careful attention to detail on the order of operations since "go" will yield control back to the browser which may decide to reflow/repaint the screen although the animation wasn't properly started yet.

Example:

(defn show-toast [message]
  (let [toast (create-toast-dom message)]
    (dom/append js/document.body toast)
    ;; X - PROBLEM: REPAINT HAPPENS NOW
    (go (<! (anim/slide-in-from-bottom animation-time toast))
        (<! (timeout 3000))
        (<! (anim/fade-out animation-time toast))
        (dom/remove toast)
        )))

This example tries to create a "Toast" [1] DOM node which is animated in from the bottom and removed after 3 seconds. A real implementation would require handling clicks and so forth but the example is easy enough to describe the problem. The (anim/...) functions set the appropriate CSS and return a (async/timeout time) channel which lets us wait for the "completion".

The problem at X is that the go is dispatched and control is given back to the browser. Therefore the "setup" part of the animation is delayed and the toast dom node is rendered by the browser as is. Usually this means that the browser will do a repaint. This will have a "flicker" effect as the node appears and then disappears again and animates (depending on the default CSS).

The simple solution is to move the dom/append call into the go block, which is easy enough in this example but hard for more complex. Another solution is to start the animation outside the go:

(defn show-toast [message]
  (let [toast (create-toast-dom message)]
    (dom/append js/document.body toast)
    (let [slide-in (anim/slide-in-from-bottom animation-time toast)]
      ;; REPAINT, but slide-in animation already started so no "flicker"
      (go (<! slide-in)
          (<! (timeout 3000))
          (<! (anim/fade-out animation-time toast))
          (dom/remove toast)
          ))))

Again, simple enough here, hard for more complex things.

What I would prefer to see is a "go!" variant which instead of dispatching the go block just starts executing it.

(defn show-toast [message]
  (let [toast (create-toast-dom message)]
    (dom/append js/document.body toast)
    (go! (<! (anim/slide-in-from-bottom animation-time toast)) ;; REPAINT ON <!
         (<! (timeout 3000))
         (<! (anim/fade-out animation-time toast))
         (dom/remove toast)
         )))

The subtle difference is that the browser does not get a chance to do anything before we are ready and would wait anyway. After fine-tuning a few of those animation effects myself it always requires moving stuff out of or into go blocks as it stands now.

I'm aware that this is a rather specific use-case and core.async might not even be the best way to do this at all but so far I had great success with it and the code is very easy to reason about.

I have not looked too much into the internals of core.async but go! should be a pretty simple addition. If this change is accepted I would start looking into things and create a patch, just wanted to make sure there were no objections before doing so.

Hope my intent is clear. CSS animations can be a bit counter-intuitive which might not make it obvious where the problem actually lies.

[1] http://www.google.com/design/spec/components/snackbars-toasts.html#snackbars-toasts-specs



 Comments   
Comment by Thomas Heller [ 06/Jul/15 6:06 AM ]

I looked into the issue to find out what needed to be done on the core.async side to facilitate this change, turns out nothing.

(defmacro go!
  "just like go, just executes immediately until the first put/take"
  [& body]
  `(let [c# (cljs.core.async/chan 1)
         f# ~(ioc/state-machine body 1 &env ioc/async-custom-terminators)
         state# (-> (f#)
                    (ioc/aset-all! cljs.core.async.impl.ioc-helpers/USER-START-IDX c#))]
     (cljs.core.async.impl.ioc-helpers/run-state-machine state#)
     c#))

Works perfectly fine so far and can live perfectly well inside a library, I'm beginning to think that this might be a better "default" for CLJS since there are no "threads" that could start running the "go".

With the normal "go" the flow of execution is:

code before go
setTimeout/nextTick
code inside go until first take/put
setTimeout/nextTick

With go! it is:

code before go!
code inside go! until first take/put
setTimeout/nextTick

Given the non-blocking nature of Javascript the extra setTimeout does not hurt it doesn't provide much benefit either.

Since no changes to core.async are required I can simply keep this macro in my library. My use-case is rather specific so feel free to close the issue if there is no interest to make this "common".

Comment by Leon Grapenthin [ 06/Jul/15 5:46 PM ]

As a default this would violate the async execution guarantee of go which would be a breaking change. Some observations from my user perspective: For readability I'd prefer both examples you proposed for usage of go over go-now because go conveys async execution of its entire body whereas in go-now I'd have to scan the body for a parkable operation to determine the part which executes synchronously. I find it difficult to convince myself that there could really be problems so complex that go-now would be preferable over refactoring go. (I don't think you can safely rely on impl namespaces in library code )





[ASYNC-106] A unit test in ioc_macros_test.clj tests nothing Created: 10/Nov/14  Updated: 06/Jul/15  Resolved: 06/Jul/15

Status: Closed
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

I am not sure what it should be doing, but this test in file ioc_macros_test.clj is always true, because it is of the form (is (= x)), which always passes:

(testing "lazy-seqs in bodies"
    (is (= (runner
            (loop []
              (when-let [x (pause 10)]
                (pause (vec (for [i (range x)]
                              i)))
                (if-not x
                  (recur))))))))


 Comments   
Comment by Alex Miller [ 06/Jul/15 4:08 PM ]

Committed a fix.





[ASYNC-124] put! into channel that has mapcat transducer fails to dispatch more than one waiting takes Created: 30/May/15  Updated: 06/Jul/15

Status: Open
Project: core.async
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Critical
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Clojure 1.7.0-RC1, latest core.async head (May 2015)


Attachments: Text File async-124-2.patch     Text File async-124-3.patch     Text File async-124.patch    
Approval: Triaged

 Description   

This issue is similar to one that has caused the revert of ASYNC-103. When channel has multiple pending takes, one put will dispatch only one take, even if transducer attached to the channel produces multiple values.

(let [xf (mapcat #(range % (+ % 5)))
        c (chan 10 xf)]
    (dotimes [x 5]
      (take! c #(println x %)))
    (put! c 10)
    (put! c 15))

The output of the above code is

0 10
1 11

but it should be

0 10
1 11
2 12
3 13
4 14

Note that if we change the order and execute puts before takes, the result will be as expected



 Comments   
Comment by Jozef Wagner [ 30/May/15 10:30 AM ]

patch with test

Comment by Jozef Wagner [ 30/May/15 10:32 AM ]

Note that with attached patch the actual dispatch/run on takes happens before (.unlock mutex) and before (when done? (abort this)) are called.

Also note that added test only works with 1.7.0 version of clojure

Comment by Jozef Wagner [ 30/May/15 12:46 PM ]

Added new patch async-124-2.patch that dispatches after unlocking and aborting. Test was also changed as it had race condition.

Comment by Alex Miller [ 06/Jul/15 9:23 AM ]

We can't add a test that is 1.7-only. See https://github.com/clojure/core.async/blob/master/src/test/clojure/clojure/core/pipeline_test.clj#L7 for an example of how to patch in a transducer impl that will work in pre-1.7.

Comment by Jozef Wagner [ 06/Jul/15 10:41 AM ]

Added async-124-3.patch that works with Clojure <1.7





Generated at Wed Jul 08 01:57:27 CDT 2015 using JIRA 4.4#649-r158309.